On Thu, Apr 06, 2017 at 04:21:50PM +0200, David Sterba wrote:
> On Wed, Apr 05, 2017 at 02:04:19PM -0700, Liu Bo wrote:
> > When doing directIO repair, we have this oops
> >
> > [ 1458.532816] general protection fault: 0000 [#1] SMP
> > ...
> > [ 1458.536291] Workqueue: btrfs-endio-repair btrfs_endio_repair_helper [btrfs]
> > [ 1458.536893] task: ffff88082a42d100 task.stack: ffffc90002b3c000
> > [ 1458.537499] RIP: 0010:btrfs_retry_endio+0x7e/0x1a0 [btrfs]
> > ...
> > [ 1458.543261] Call Trace:
> > [ 1458.543958] ? rcu_read_lock_sched_held+0xc4/0xd0
> > [ 1458.544374] bio_endio+0xed/0x100
> > [ 1458.544750] end_workqueue_fn+0x3c/0x40 [btrfs]
> > [ 1458.545257] normal_work_helper+0x9f/0x900 [btrfs]
> > [ 1458.545762] btrfs_endio_repair_helper+0x12/0x20 [btrfs]
> > [ 1458.546224] process_one_work+0x34d/0xb70
> > [ 1458.546570] ? process_one_work+0x29e/0xb70
> > [ 1458.546938] worker_thread+0x1cf/0x960
> > [ 1458.547263] ? process_one_work+0xb70/0xb70
> > [ 1458.547624] kthread+0x17d/0x180
> > [ 1458.547909] ? kthread_create_on_node+0x70/0x70
> > [ 1458.548300] ret_from_fork+0x31/0x40
> >
> > It turns out that btrfs_retry_endio is trying to get inode from a directIO
> > page.
> >
> > This fixes the problem by using the preservd inode pointer, done->inode.
> > btrfs_retry_endio_nocsum has the same problem, and it's fixed as well.
> >
> > Also cleanup unused @start(which is too trivial for a separate patch).
> >
> > Cc: David Sterba <dsterba@xxxxxxx>
> > Signed-off-by: Liu Bo <bo.li.liu@xxxxxxxxxx>
>
> Reviewed-by: David Sterba <dsterba@xxxxxxxx>
>
> > ---
> > fs/btrfs/inode.c | 14 ++++----------
> > 1 file changed, 4 insertions(+), 10 deletions(-)
> >
> > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > index 3ec5a05..8e71ed7 100644
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -7910,7 +7910,6 @@ struct btrfs_retry_complete {
> > static void btrfs_retry_endio_nocsum(struct bio *bio)
> > {
> > struct btrfs_retry_complete *done = bio->bi_private;
> > - struct inode *inode;
> > struct bio_vec *bvec;
> > int i;
> >
> > @@ -7918,12 +7917,12 @@ static void btrfs_retry_endio_nocsum(struct bio *bio)
> > goto end;
> >
> > ASSERT(bio->bi_vcnt == 1);
> > - inode = bio->bi_io_vec->bv_page->mapping->host;
> > - ASSERT(bio->bi_io_vec->bv_len == btrfs_inode_sectorsize(inode));
> > + ASSERT(bio->bi_io_vec->bv_len == btrfs_inode_sectorsize(done->inode));
>
> I was interested how it got here in the first place, inode from mapping
> hasn't been used in the initial support for DIO repair. So it was added
> in the subpage blocksize preparation patchset (2dabb3248453b), slipped
> through the review.
Yes, it's a regression, and I've found another regression in this commit.
Thanks,
-liubo
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
