| Message ID | 20170929155030.7726-1-rgoldwyn@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Sep 29, 2017 at 10:50:30AM -0500, Goldwyn Rodrigues wrote: > In case direct I/O encounters an error midway, it returns the error. > Instead it should be returning the number of bytes transferred so far. > > Test case for filesystems (with ENOSPC): > 1. Create an almost full filesystem > 2. Create a file, say /mnt/lastfile, until the filesystem is full. > 3. Direct write() with count > sizeof /mnt/lastfile. > > Result: write() returns -ENOSPC. However, file content has data written > in step 3. > > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> > > Changes since v1: > - incorporated iomap and block devices > > Changes since v2: > - realized that file size was not increasing when performing a (partial) > direct I/O because end_io function was receiving the error instead of > size in iomap's dio end io. Fixed. > > diff --git a/fs/block_dev.c b/fs/block_dev.c > index 93d088ffc05c..1c6640ffb929 100644 > --- a/fs/block_dev.c > +++ b/fs/block_dev.c > @@ -421,7 +421,7 @@ __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, int nr_pages) > > if (!ret) > ret = blk_status_to_errno(dio->bio.bi_status); > - if (likely(!ret)) > + if (likely(dio->size)) > ret = dio->size; > > bio_put(&dio->bio); > diff --git a/fs/direct-io.c b/fs/direct-io.c > index 5fa2211e49ae..0fc1789498ae 100644 > --- a/fs/direct-io.c > +++ b/fs/direct-io.c > @@ -255,8 +255,6 @@ static ssize_t dio_complete(struct dio *dio, ssize_t ret, bool is_async) > ret = dio->page_errors; > if (ret == 0) > ret = dio->io_error; > - if (ret == 0) > - ret = transferred; > > if (dio->end_io) { > int err; > @@ -284,7 +282,7 @@ static ssize_t dio_complete(struct dio *dio, ssize_t ret, bool is_async) > } > > kmem_cache_free(dio_cache, dio); > - return ret; > + return transferred ? transferred : ret; > } > > static void dio_aio_complete_work(struct work_struct *work) > diff --git a/fs/iomap.c b/fs/iomap.c > index 269b24a01f32..019c4d993300 100644 > --- a/fs/iomap.c > +++ b/fs/iomap.c > @@ -714,28 +714,29 @@ static ssize_t iomap_dio_complete(struct iomap_dio *dio) > { > struct kiocb *iocb = dio->iocb; > ssize_t ret; > + ssize_t transferred = 0; How about initializing this to dio->size here and then using it further down everywere you currently use dio->size? Otherwise looks fine: Reviewed-by: Christoph Hellwig <hch@lst.de>
diff --git a/fs/block_dev.c b/fs/block_dev.c index 93d088ffc05c..1c6640ffb929 100644 --- a/fs/block_dev.c +++ b/fs/block_dev.c @@ -421,7 +421,7 @@ __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, int nr_pages) if (!ret) ret = blk_status_to_errno(dio->bio.bi_status); - if (likely(!ret)) + if (likely(dio->size)) ret = dio->size; bio_put(&dio->bio); diff --git a/fs/direct-io.c b/fs/direct-io.c index 5fa2211e49ae..0fc1789498ae 100644 --- a/fs/direct-io.c +++ b/fs/direct-io.c @@ -255,8 +255,6 @@ static ssize_t dio_complete(struct dio *dio, ssize_t ret, bool is_async) ret = dio->page_errors; if (ret == 0) ret = dio->io_error; - if (ret == 0) - ret = transferred; if (dio->end_io) { int err; @@ -284,7 +282,7 @@ static ssize_t dio_complete(struct dio *dio, ssize_t ret, bool is_async) } kmem_cache_free(dio_cache, dio); - return ret; + return transferred ? transferred : ret; } static void dio_aio_complete_work(struct work_struct *work) diff --git a/fs/iomap.c b/fs/iomap.c index 269b24a01f32..019c4d993300 100644 --- a/fs/iomap.c +++ b/fs/iomap.c @@ -714,28 +714,29 @@ static ssize_t iomap_dio_complete(struct iomap_dio *dio) { struct kiocb *iocb = dio->iocb; ssize_t ret; + ssize_t transferred = 0; if (dio->end_io) { ret = dio->end_io(iocb, - dio->error ? dio->error : dio->size, + dio->size ? dio->size : dio->error, dio->flags); } else { ret = dio->error; } - if (likely(!ret)) { - ret = dio->size; + if (likely(dio->size)) { + transferred = dio->size; /* check for short read */ - if (iocb->ki_pos + ret > dio->i_size && + if (iocb->ki_pos + transferred > dio->i_size && !(dio->flags & IOMAP_DIO_WRITE)) - ret = dio->i_size - iocb->ki_pos; - iocb->ki_pos += ret; + transferred = dio->i_size - iocb->ki_pos; + iocb->ki_pos += transferred; } inode_dio_end(file_inode(iocb->ki_filp)); kfree(dio); - return ret; + return transferred ? transferred : ret; } static void iomap_dio_complete_work(struct work_struct *work)
In case direct I/O encounters an error midway, it returns the error. Instead it should be returning the number of bytes transferred so far. Test case for filesystems (with ENOSPC): 1. Create an almost full filesystem 2. Create a file, say /mnt/lastfile, until the filesystem is full. 3. Direct write() with count > sizeof /mnt/lastfile. Result: write() returns -ENOSPC. However, file content has data written in step 3. Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> Changes since v1: - incorporated iomap and block devices Changes since v2: - realized that file size was not increasing when performing a (partial) direct I/O because end_io function was receiving the error instead of size in iomap's dio end io. Fixed.