[PATCH] Return short read or 0 at end of a raw device, not EIO

[PATCH] Return short read or 0 at end of a raw device, not EIO

[David Jeffery]

Changes to the basic direct I/O code have broken the raw driver when reading
to the end of a raw device.  Instead of returning a short read for a read that
extends partially beyond the device's end or 0 when at the end of the device,
these reads now return EIO.

The raw driver needs the same end of device handling as was added for normal
block devices.  Using blkdev_read_iter, which has the needed size checks,
prevents the EIO conditions at the end of the device.

Signed-off-by: David Jeffery <djeffery@redhat.com>
---

 drivers/char/raw.c |    2 +-
 fs/block_dev.c     |    3 ++-
 include/linux/fs.h |    1 +
 3 files changed, 4 insertions(+), 2 deletions(-)


diff --git a/drivers/char/raw.c b/drivers/char/raw.c
index 0102dc7..a24891b 100644
--- a/drivers/char/raw.c
+++ b/drivers/char/raw.c
@@ -285,7 +285,7 @@ static long raw_ctl_compat_ioctl(struct file *file, unsigned int cmd,
 
 static const struct file_operations raw_fops = {
 	.read		= new_sync_read,
-	.read_iter	= generic_file_read_iter,
+	.read_iter	= blkdev_read_iter,
 	.write		= new_sync_write,
 	.write_iter	= blkdev_write_iter,
 	.fsync		= blkdev_fsync,
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 6d72746..12aa041 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1591,7 +1591,7 @@ ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
 }
 EXPORT_SYMBOL_GPL(blkdev_write_iter);
 
-static ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to)
+ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to)
 {
 	struct file *file = iocb->ki_filp;
 	struct inode *bd_inode = file->f_mapping->host;
@@ -1605,6 +1605,7 @@ static ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to)
 	iov_iter_truncate(to, size);
 	return generic_file_read_iter(iocb, to);
 }
+EXPORT_SYMBOL_GPL(blkdev_read_iter);
 
 /*
  * Try to release a page associated with block device when the system
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 9418772..0c20168 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2455,6 +2455,7 @@ extern ssize_t new_sync_read(struct file *filp, char __user *buf, size_t len, lo
 extern ssize_t new_sync_write(struct file *filp, const char __user *buf, size_t len, loff_t *ppos);
 
 /* fs/block_dev.c */
+extern ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to);
 extern ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from);
 extern int blkdev_fsync(struct file *filp, loff_t start, loff_t end,
 			int datasync);

Re: [PATCH] Return short read or 0 at end of a raw device, not EIO

[Jeff Moyer]

David Jeffery <djeffery@redhat.com> writes:

> Changes to the basic direct I/O code have broken the raw driver when reading
> to the end of a raw device.  Instead of returning a short read for a read that
> extends partially beyond the device's end or 0 when at the end of the device,
> these reads now return EIO.
>
> The raw driver needs the same end of device handling as was added for normal
> block devices.  Using blkdev_read_iter, which has the needed size checks,
> prevents the EIO conditions at the end of the device.
>
> Signed-off-by: David Jeffery <djeffery@redhat.com>

Looks good to me.

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

> ---
>
>  drivers/char/raw.c |    2 +-
>  fs/block_dev.c     |    3 ++-
>  include/linux/fs.h |    1 +
>  3 files changed, 4 insertions(+), 2 deletions(-)
>
>
> diff --git a/drivers/char/raw.c b/drivers/char/raw.c
> index 0102dc7..a24891b 100644
> --- a/drivers/char/raw.c
> +++ b/drivers/char/raw.c
> @@ -285,7 +285,7 @@ static long raw_ctl_compat_ioctl(struct file *file, unsigned int cmd,
>  
>  static const struct file_operations raw_fops = {
>  	.read		= new_sync_read,
> -	.read_iter	= generic_file_read_iter,
> +	.read_iter	= blkdev_read_iter,
>  	.write		= new_sync_write,
>  	.write_iter	= blkdev_write_iter,
>  	.fsync		= blkdev_fsync,
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 6d72746..12aa041 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1591,7 +1591,7 @@ ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  }
>  EXPORT_SYMBOL_GPL(blkdev_write_iter);
>  
> -static ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to)
> +ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to)
>  {
>  	struct file *file = iocb->ki_filp;
>  	struct inode *bd_inode = file->f_mapping->host;
> @@ -1605,6 +1605,7 @@ static ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to)
>  	iov_iter_truncate(to, size);
>  	return generic_file_read_iter(iocb, to);
>  }
> +EXPORT_SYMBOL_GPL(blkdev_read_iter);
>  
>  /*
>   * Try to release a page associated with block device when the system
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 9418772..0c20168 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2455,6 +2455,7 @@ extern ssize_t new_sync_read(struct file *filp, char __user *buf, size_t len, lo
>  extern ssize_t new_sync_write(struct file *filp, const char __user *buf, size_t len, loff_t *ppos);
>  
>  /* fs/block_dev.c */
> +extern ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to);
>  extern ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from);
>  extern int blkdev_fsync(struct file *filp, loff_t start, loff_t end,
>  			int datasync);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Return short read or 0 at end of a raw device, not EIO

[Christoph Hellwig]

On Mon, Sep 29, 2014 at 10:21:10AM -0400, David Jeffery wrote:
> Changes to the basic direct I/O code have broken the raw driver when reading
> to the end of a raw device.  Instead of returning a short read for a read that
> extends partially beyond the device's end or 0 when at the end of the device,
> these reads now return EIO.

Seems like this should be changed in the generic code, or is there some
reason why it would return EIO only for devices, but not for regular
files in this case?

Re: [PATCH] Return short read or 0 at end of a raw device, not EIO

[David Jeffery]

On 09/29/2014 03:05 PM, Christoph Hellwig wrote:
> Seems like this should be changed in the generic code, or is there some
> reason why it would return EIO only for devices, but not for regular
> files in this case?
> 

Regular files shouldn't be returning EIO and don't in my tests. The file
systems manage direct I/O EOF handling in their own block or direct_IO
callbacks.  Block devices do not and instead do the size checks up
front.  Raw devices were bypassing the block device check, so only the
raw driver should be having this issue.

David Jeffery

Re: [PATCH] Return short read or 0 at end of a raw device, not EIO

[Christoph Hellwig]

On Mon, Sep 29, 2014 at 06:08:11PM -0400, David Jeffery wrote:
> On 09/29/2014 03:05 PM, Christoph Hellwig wrote:
> > Seems like this should be changed in the generic code, or is there some
> > reason why it would return EIO only for devices, but not for regular
> > files in this case?
> > 
> 
> Regular files shouldn't be returning EIO and don't in my tests. The file
> systems manage direct I/O EOF handling in their own block or direct_IO
> callbacks.  Block devices do not and instead do the size checks up
> front.  Raw devices were bypassing the block device check, so only the
> raw driver should be having this issue.

So I guess the problem is commit

"blkdev_aio_read(): switch to generic_file_read_iter(), get rid of iov_shorten()"

which removes the iov_shorten call in blkdev_aio_read?  This should be
mentioned in the changelog.

But maybe we should instead make block devices behave more similar to
regular files in this respect?

Also did you make sure to add your regression test somewhere, e.g. ltp?

Re: [PATCH] Return short read or 0 at end of a raw device, not EIO

[David Jeffery]

On 09/30/2014 11:28 AM, Christoph Hellwig wrote:
> On Mon, Sep 29, 2014 at 06:08:11PM -0400, David Jeffery wrote:
>> On 09/29/2014 03:05 PM, Christoph Hellwig wrote:
>>> Seems like this should be changed in the generic code, or is there some
>>> reason why it would return EIO only for devices, but not for regular
>>> files in this case?
>>>
>>
>> Regular files shouldn't be returning EIO and don't in my tests. The file
>> systems manage direct I/O EOF handling in their own block or direct_IO
>> callbacks.  Block devices do not and instead do the size checks up
>> front.  Raw devices were bypassing the block device check, so only the
>> raw driver should be having this issue.
> 
> So I guess the problem is commit
> 
> "blkdev_aio_read(): switch to generic_file_read_iter(), get rid of iov_shorten()"
> 
> which removes the iov_shorten call in blkdev_aio_read?  This should be
> mentioned in the changelog.
> 
> But maybe we should instead make block devices behave more similar to
> regular files in this respect?

The issue's origin should be with bbec0270bdd8 "blkdev_max_block: make
private to fs/buffer.c".  This change intentionally removed the size
checks from blkdev_get_block() to avoid issues with the size variables
changing underneath the calls.  Commit 684c9aaebbb0 is the initial
commit to add the size checks with blkdev_aio_read() since
blkdev_get_block() no longer checks size.  As the raw driver was also
dependent on the size checks in blkdev_get_block(), it was broken by
bbec0270bdd8 but not fixed at all by 684c9aaebbb0.

> 
> Also did you make sure to add your regression test somewhere, e.g. ltp?
> 

No I have not.  I'll see about an ltp test.

Re: [PATCH] Return short read or 0 at end of a raw device, not EIO

[Jeff Moyer]

David Jeffery <djeffery@redhat.com> writes:

> On 09/30/2014 11:28 AM, Christoph Hellwig wrote:
>> On Mon, Sep 29, 2014 at 06:08:11PM -0400, David Jeffery wrote:
>>> On 09/29/2014 03:05 PM, Christoph Hellwig wrote:
>>>> Seems like this should be changed in the generic code, or is there some
>>>> reason why it would return EIO only for devices, but not for regular
>>>> files in this case?
>>>>
>>>
>>> Regular files shouldn't be returning EIO and don't in my tests. The file
>>> systems manage direct I/O EOF handling in their own block or direct_IO
>>> callbacks.  Block devices do not and instead do the size checks up
>>> front.  Raw devices were bypassing the block device check, so only the
>>> raw driver should be having this issue.
>> 
>> So I guess the problem is commit
>> 
>> "blkdev_aio_read(): switch to generic_file_read_iter(), get rid of iov_shorten()"
>> 
>> which removes the iov_shorten call in blkdev_aio_read?  This should be
>> mentioned in the changelog.
>> 
>> But maybe we should instead make block devices behave more similar to
>> regular files in this respect?
>
> The issue's origin should be with bbec0270bdd8 "blkdev_max_block: make
> private to fs/buffer.c".  This change intentionally removed the size
> checks from blkdev_get_block() to avoid issues with the size variables
> changing underneath the calls.  Commit 684c9aaebbb0 is the initial
> commit to add the size checks with blkdev_aio_read() since
> blkdev_get_block() no longer checks size.  As the raw driver was also
> dependent on the size checks in blkdev_get_block(), it was broken by
> bbec0270bdd8 but not fixed at all by 684c9aaebbb0.
>
>> 
>> Also did you make sure to add your regression test somewhere, e.g. ltp?
>> 
>
> No I have not.  I'll see about an ltp test.

Christoph, do you have any issues with the patch as it stands?  Whose
tree do you think this should go through?  akpm's?

Cheers,
Jeff

Re: [PATCH] Return short read or 0 at end of a raw device, not EIO

[Christoph Hellwig]

On Fri, Oct 10, 2014 at 01:17:28PM -0400, Jeff Moyer wrote:
> Christoph, do you have any issues with the patch as it stands?  Whose
> tree do you think this should go through?  akpm's?

It looks good.  I think Al's tree should be fine.

Re: [PATCH] Return short read or 0 at end of a raw device, not EIO

[Christoph Hellwig]

Could someone please pick up this regression fix?

On Mon, Sep 29, 2014 at 10:21:10AM -0400, David Jeffery wrote:
> Changes to the basic direct I/O code have broken the raw driver when reading
> to the end of a raw device.  Instead of returning a short read for a read that
> extends partially beyond the device's end or 0 when at the end of the device,
> these reads now return EIO.
> 
> The raw driver needs the same end of device handling as was added for normal
> block devices.  Using blkdev_read_iter, which has the needed size checks,
> prevents the EIO conditions at the end of the device.
> 
> Signed-off-by: David Jeffery <djeffery@redhat.com>
> ---
> 
>  drivers/char/raw.c |    2 +-
>  fs/block_dev.c     |    3 ++-
>  include/linux/fs.h |    1 +
>  3 files changed, 4 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/drivers/char/raw.c b/drivers/char/raw.c
> index 0102dc7..a24891b 100644
> --- a/drivers/char/raw.c
> +++ b/drivers/char/raw.c
> @@ -285,7 +285,7 @@ static long raw_ctl_compat_ioctl(struct file *file, unsigned int cmd,
>  
>  static const struct file_operations raw_fops = {
>  	.read		= new_sync_read,
> -	.read_iter	= generic_file_read_iter,
> +	.read_iter	= blkdev_read_iter,
>  	.write		= new_sync_write,
>  	.write_iter	= blkdev_write_iter,
>  	.fsync		= blkdev_fsync,
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 6d72746..12aa041 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1591,7 +1591,7 @@ ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  }
>  EXPORT_SYMBOL_GPL(blkdev_write_iter);
>  
> -static ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to)
> +ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to)
>  {
>  	struct file *file = iocb->ki_filp;
>  	struct inode *bd_inode = file->f_mapping->host;
> @@ -1605,6 +1605,7 @@ static ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to)
>  	iov_iter_truncate(to, size);
>  	return generic_file_read_iter(iocb, to);
>  }
> +EXPORT_SYMBOL_GPL(blkdev_read_iter);
>  
>  /*
>   * Try to release a page associated with block device when the system
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 9418772..0c20168 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2455,6 +2455,7 @@ extern ssize_t new_sync_read(struct file *filp, char __user *buf, size_t len, lo
>  extern ssize_t new_sync_write(struct file *filp, const char __user *buf, size_t len, loff_t *ppos);
>  
>  /* fs/block_dev.c */
> +extern ssize_t blkdev_read_iter(struct kiocb *iocb, struct iov_iter *to);
>  extern ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from);
>  extern int blkdev_fsync(struct file *filp, loff_t start, loff_t end,
>  			int datasync);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
---end quoted text---

Re: [PATCH] Return short read or 0 at end of a raw device, not EIO

[Al Viro]

On Fri, Oct 31, 2014 at 01:57:22AM -0700, Christoph Hellwig wrote:
> Could someone please pick up this regression fix?

Done.