Message ID | 20190411064549.13318-1-yanaijie@huawei.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | block: fix the return errno for direct IO | expand |
On 4/10/19 11:28 PM, Jason Yan wrote: > If the last bio returned is not dio->bio, the status of the bio will > not assigned to dio->bio if it is error. This will cause the whole IO > status wrong. > > ksoftirqd/21-117 [021] ..s. 4017.966090: 8,0 C N 4883648 [0] > <idle>-0 [018] ..s. 4017.970888: 8,0 C WS 4924800 + 1024 [0] > <idle>-0 [018] ..s. 4017.970909: 8,0 D WS 4935424 + 1024 [<idle>] > <idle>-0 [018] ..s. 4017.970924: 8,0 D WS 4936448 + 321 [<idle>] > ksoftirqd/21-117 [021] ..s. 4017.995033: 8,0 C R 4883648 + 336 [65475] > ksoftirqd/21-117 [021] d.s. 4018.001988: myprobe1: (blkdev_bio_end_io+0x0/0x168) bi_status=7 > ksoftirqd/21-117 [021] d.s. 4018.001992: myprobe: (aio_complete_rw+0x0/0x148) x0=0xffff802f2595ad80 res=0x12a000 res2=0x0 > > We always have to assign bio->bi_status to dio->bio.bi_status because we > will only check dio->bio.bi_status when we return the whole IO to > the upper layer. > > Fixes: 542ff7bf18c6 ("block: new direct I/O implementation") > Cc: Christoph Hellwig <hch@lst.de> > Cc: Jens Axboe <axboe@kernel.dk> > Cc: Ming Lei <ming.lei@redhat.com> > Signed-off-by: Jason Yan <yanaijie@huawei.com> > --- > fs/block_dev.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/fs/block_dev.c b/fs/block_dev.c > index 78d3257435c0..0fead5520a7e 100644 > --- a/fs/block_dev.c > +++ b/fs/block_dev.c > @@ -84,7 +84,7 @@ void kill_bdev(struct block_device *bdev) > > invalidate_bh_lrus(); > truncate_inode_pages(mapping, 0); > -} > +} Didn't understand above change, is it possible to get rid of above line change ? > EXPORT_SYMBOL(kill_bdev); > > /* Invalidate clean unused buffers and pagecache. */ > @@ -307,10 +307,10 @@ static void blkdev_bio_end_io(struct bio *bio) > struct blkdev_dio *dio = bio->bi_private; > bool should_dirty = dio->should_dirty; > > - if (dio->multi_bio && !atomic_dec_and_test(&dio->ref)) { > - if (bio->bi_status && !dio->bio.bi_status) > - dio->bio.bi_status = bio->bi_status; > - } else { > + if (bio->bi_status && !dio->bio.bi_status) > + dio->bio.bi_status = bio->bi_status; > + > + if (!dio->multi_bio || atomic_dec_and_test(&dio->ref)) { > if (!dio->is_sync) { > struct kiocb *iocb = dio->iocb; > ssize_t ret; > @@ -670,13 +670,13 @@ static loff_t block_llseek(struct file *file, loff_t offset, int whence) > inode_unlock(bd_inode); > return retval; > } > - > + Same here. > int blkdev_fsync(struct file *filp, loff_t start, loff_t end, int datasync) > { > struct inode *bd_inode = bdev_file_inode(filp); > struct block_device *bdev = I_BDEV(bd_inode); > int error; > - > + Same here. > error = file_write_and_wait_range(filp, start, end); > if (error) > return error; > @@ -983,7 +983,7 @@ void bdput(struct block_device *bdev) > } > > EXPORT_SYMBOL(bdput); > - > + Same here. > static struct block_device *bd_acquire(struct inode *inode) > { > struct block_device *bdev; >
Hi Chaitanya, On 2019/4/11 14:57, Chaitanya Kulkarni wrote: >> diff --git a/fs/block_dev.c b/fs/block_dev.c >> index 78d3257435c0..0fead5520a7e 100644 >> --- a/fs/block_dev.c >> +++ b/fs/block_dev.c >> @@ -84,7 +84,7 @@ void kill_bdev(struct block_device *bdev) >> >> invalidate_bh_lrus(); >> truncate_inode_pages(mapping, 0); >> -} >> +} > Didn't understand above change, is it possible to get rid of above line > change ? Hi, there are some extra white space at the end of the line and my editor removed those white space. If people don't like this change, I can remove it. Thanks, Jason
diff --git a/fs/block_dev.c b/fs/block_dev.c index 78d3257435c0..0fead5520a7e 100644 --- a/fs/block_dev.c +++ b/fs/block_dev.c @@ -84,7 +84,7 @@ void kill_bdev(struct block_device *bdev) invalidate_bh_lrus(); truncate_inode_pages(mapping, 0); -} +} EXPORT_SYMBOL(kill_bdev); /* Invalidate clean unused buffers and pagecache. */ @@ -307,10 +307,10 @@ static void blkdev_bio_end_io(struct bio *bio) struct blkdev_dio *dio = bio->bi_private; bool should_dirty = dio->should_dirty; - if (dio->multi_bio && !atomic_dec_and_test(&dio->ref)) { - if (bio->bi_status && !dio->bio.bi_status) - dio->bio.bi_status = bio->bi_status; - } else { + if (bio->bi_status && !dio->bio.bi_status) + dio->bio.bi_status = bio->bi_status; + + if (!dio->multi_bio || atomic_dec_and_test(&dio->ref)) { if (!dio->is_sync) { struct kiocb *iocb = dio->iocb; ssize_t ret; @@ -670,13 +670,13 @@ static loff_t block_llseek(struct file *file, loff_t offset, int whence) inode_unlock(bd_inode); return retval; } - + int blkdev_fsync(struct file *filp, loff_t start, loff_t end, int datasync) { struct inode *bd_inode = bdev_file_inode(filp); struct block_device *bdev = I_BDEV(bd_inode); int error; - + error = file_write_and_wait_range(filp, start, end); if (error) return error; @@ -983,7 +983,7 @@ void bdput(struct block_device *bdev) } EXPORT_SYMBOL(bdput); - + static struct block_device *bd_acquire(struct inode *inode) { struct block_device *bdev;
If the last bio returned is not dio->bio, the status of the bio will not assigned to dio->bio if it is error. This will cause the whole IO status wrong. ksoftirqd/21-117 [021] ..s. 4017.966090: 8,0 C N 4883648 [0] <idle>-0 [018] ..s. 4017.970888: 8,0 C WS 4924800 + 1024 [0] <idle>-0 [018] ..s. 4017.970909: 8,0 D WS 4935424 + 1024 [<idle>] <idle>-0 [018] ..s. 4017.970924: 8,0 D WS 4936448 + 321 [<idle>] ksoftirqd/21-117 [021] ..s. 4017.995033: 8,0 C R 4883648 + 336 [65475] ksoftirqd/21-117 [021] d.s. 4018.001988: myprobe1: (blkdev_bio_end_io+0x0/0x168) bi_status=7 ksoftirqd/21-117 [021] d.s. 4018.001992: myprobe: (aio_complete_rw+0x0/0x148) x0=0xffff802f2595ad80 res=0x12a000 res2=0x0 We always have to assign bio->bi_status to dio->bio.bi_status because we will only check dio->bio.bi_status when we return the whole IO to the upper layer. Fixes: 542ff7bf18c6 ("block: new direct I/O implementation") Cc: Christoph Hellwig <hch@lst.de> Cc: Jens Axboe <axboe@kernel.dk> Cc: Ming Lei <ming.lei@redhat.com> Signed-off-by: Jason Yan <yanaijie@huawei.com> --- fs/block_dev.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)