| Message ID | 1491426259-27838-1-git-send-email-bo.li.liu@oracle.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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@suse.cz> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com> Reviewed-by: David Sterba <dsterba@suse.com> > --- > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@suse.cz> > > Signed-off-by: Liu Bo <bo.li.liu@oracle.com> > > Reviewed-by: David Sterba <dsterba@suse.com> > > > --- > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)); done->uptodate = 1; bio_for_each_segment_all(bvec, bio, i) - clean_io_failure(BTRFS_I(done->inode), done->start, bvec->bv_page, 0); + clean_io_failure(BTRFS_I(done->inode), done->start, + bvec->bv_page, 0); end: complete(&done->done); bio_put(bio); @@ -7986,9 +7985,7 @@ static void btrfs_retry_endio(struct bio *bio) { struct btrfs_retry_complete *done = bio->bi_private; struct btrfs_io_bio *io_bio = btrfs_io_bio(bio); - struct inode *inode; struct bio_vec *bvec; - u64 start; int uptodate; int ret; int i; @@ -7998,11 +7995,8 @@ static void btrfs_retry_endio(struct bio *bio) uptodate = 1; - start = done->start; - 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)); bio_for_each_segment_all(bvec, bio, i) { ret = __readpage_endio_check(done->inode, io_bio, i,
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@suse.cz> Signed-off-by: Liu Bo <bo.li.liu@oracle.com> --- fs/btrfs/inode.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)