[PATCH v7 1/2] xfs: remove assert to check bytes returned

[PATCH v7 1/2] xfs: remove assert to check bytes returned

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

Since we can return less than count in case of partial direct
writes, remove the ASSERT.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/xfs/xfs_file.c | 6 ------
 1 file changed, 6 deletions(-)

Changes since v6:
 - Reordered to before direct write fix

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 8601275cc5e6..8fc4dbf66910 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -590,12 +590,6 @@ xfs_file_dio_aio_write(
 	ret = iomap_dio_rw(iocb, from, &xfs_iomap_ops, xfs_dio_write_end_io);
 out:
 	xfs_iunlock(ip, iolock);
-
-	/*
-	 * No fallback to buffered IO on errors for XFS, direct IO will either
-	 * complete fully or fail.
-	 */
-	ASSERT(ret < 0 || ret == count);
 	return ret;
 }
 
-- 
2.16.1

[PATCH v7 2/2] Return bytes transferred for partial direct I/O

[Goldwyn Rodrigues]

From: Goldwyn Rodrigues <rgoldwyn@suse.com>

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.

Added a sysctl entry: dio_short_writes which is on by default. This is
to support applications which expect either and error or the bytes submitted
as a return value for the write calls.

This fixes fstest generic/472.

Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 Documentation/sysctl/fs.txt | 14 ++++++++++++++
 fs/block_dev.c              |  2 +-
 fs/direct-io.c              |  7 +++++--
 fs/iomap.c                  | 23 ++++++++++++-----------
 include/linux/fs.h          |  1 +
 kernel/sysctl.c             |  9 +++++++++
 6 files changed, 42 insertions(+), 14 deletions(-)

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. Fixed.

Changes since v3:
 - [hch] initialize transferred with dio->size and use transferred instead
   of dio->size.

Changes since v4:
 - Refreshed to v4.14

Changes since v5:
 - Added /proc/sys/fs/dio_short_writes (default 1) to guard older applications
   which expect write(fd, buf, count) returns either count or error.

Changes since v6:
 - Corrected documentation
 - Re-ordered patch

diff --git a/Documentation/sysctl/fs.txt b/Documentation/sysctl/fs.txt
index 6c00c1e2743f..21582f675985 100644
--- a/Documentation/sysctl/fs.txt
+++ b/Documentation/sysctl/fs.txt
@@ -22,6 +22,7 @@ Currently, these files are in /proc/sys/fs:
 - aio-max-nr
 - aio-nr
 - dentry-state
+- dio_short_writes
 - dquot-max
 - dquot-nr
 - file-max
@@ -76,6 +77,19 @@ dcache isn't pruned yet.
 
 ==============================================================
 
+dio_short_writes:
+
+In case Direct I/O encounters a transient error, it returns
+the error code, even if it has performed part of the write.
+This flag, if on (default), will return the number of bytes written
+so far, as the write(2) semantics are. However, some older applications
+still consider a direct write as an error if all of the I/O
+submitted is not complete. I.e. write(file, count, buf) != count.
+This option can be disabled on systems in order to support
+existing applications which do not expect short writes.
+
+==============================================================
+
 dquot-max & dquot-nr:
 
 The file dquot-max shows the maximum number of cached disk
diff --git a/fs/block_dev.c b/fs/block_dev.c
index 4a181fcb5175..49d94360bb51 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -409,7 +409,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 3aafb3343a65..9bd15be64c25 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -151,6 +151,7 @@ struct dio {
 } ____cacheline_aligned_in_smp;
 
 static struct kmem_cache *dio_cache __read_mostly;
+unsigned int sysctl_dio_short_writes = 1;
 
 /*
  * How many pages are in the queue?
@@ -262,7 +263,7 @@ static ssize_t dio_complete(struct dio *dio, ssize_t ret, unsigned int flags)
 		ret = dio->page_errors;
 	if (ret == 0)
 		ret = dio->io_error;
-	if (ret == 0)
+	if (!sysctl_dio_short_writes && (ret == 0))
 		ret = transferred;
 
 	if (dio->end_io) {
@@ -310,7 +311,9 @@ static ssize_t dio_complete(struct dio *dio, ssize_t ret, unsigned int flags)
 	}
 
 	kmem_cache_free(dio_cache, dio);
-	return ret;
+	if (!sysctl_dio_short_writes)
+		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 47d29ccffaef..a8d6908dc0de 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -716,23 +716,24 @@ static ssize_t iomap_dio_complete(struct iomap_dio *dio)
 	struct kiocb *iocb = dio->iocb;
 	struct inode *inode = file_inode(iocb->ki_filp);
 	loff_t offset = iocb->ki_pos;
-	ssize_t ret;
+	ssize_t err;
+	ssize_t transferred = dio->size;
 
 	if (dio->end_io) {
-		ret = dio->end_io(iocb,
-				dio->error ? dio->error : dio->size,
-				dio->flags);
+		err = dio->end_io(iocb,
+				  (transferred && sysctl_dio_short_writes) ?
+						transferred : dio->error,
+				  dio->flags);
 	} else {
-		ret = dio->error;
+		err = dio->error;
 	}
 
-	if (likely(!ret)) {
-		ret = dio->size;
+	if (likely(transferred)) {
 		/* check for short read */
-		if (offset + ret > dio->i_size &&
+		if (offset + transferred > dio->i_size &&
 		    !(dio->flags & IOMAP_DIO_WRITE))
-			ret = dio->i_size - offset;
-		iocb->ki_pos += ret;
+			transferred = dio->i_size - offset;
+		iocb->ki_pos += transferred;
 	}
 
 	/*
@@ -759,7 +760,7 @@ static ssize_t iomap_dio_complete(struct iomap_dio *dio)
 	inode_dio_end(file_inode(iocb->ki_filp));
 	kfree(dio);
 
-	return ret;
+	return (transferred && sysctl_dio_short_writes) ? transferred : err;
 }
 
 static void iomap_dio_complete_work(struct work_struct *work)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 511fbaabf624..a25652e5ae1b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1469,6 +1469,7 @@ static inline void i_gid_write(struct inode *inode, gid_t gid)
 }
 
 extern struct timespec current_time(struct inode *inode);
+extern unsigned int sysctl_dio_short_writes;
 
 /*
  * Snapshotting support.
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 557d46728577..362a9c3156f1 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1844,6 +1844,15 @@ static struct ctl_table fs_table[] = {
 		.proc_handler	= proc_dointvec_minmax,
 		.extra1		= &one,
 	},
+	{
+		.procname	= "dio_short_writes",
+		.data		= &sysctl_dio_short_writes,
+		.maxlen		= sizeof(int),
+		.mode		= 0600,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= &zero,
+		.extra2		= &one,
+	},
 	{ }
 };
 
-- 
2.16.1

Re: [PATCH v7 2/2] Return bytes transferred for partial direct I/O

[Darrick J. Wong]

On Thu, Feb 08, 2018 at 12:59:48PM -0600, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> 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.
> 
> Added a sysctl entry: dio_short_writes which is on by default. This is
> to support applications which expect either and error or the bytes submitted
> as a return value for the write calls.
> 
> This fixes fstest generic/472.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  Documentation/sysctl/fs.txt | 14 ++++++++++++++
>  fs/block_dev.c              |  2 +-
>  fs/direct-io.c              |  7 +++++--
>  fs/iomap.c                  | 23 ++++++++++++-----------
>  include/linux/fs.h          |  1 +
>  kernel/sysctl.c             |  9 +++++++++
>  6 files changed, 42 insertions(+), 14 deletions(-)
> 
> 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. Fixed.
> 
> Changes since v3:
>  - [hch] initialize transferred with dio->size and use transferred instead
>    of dio->size.
> 
> Changes since v4:
>  - Refreshed to v4.14
> 
> Changes since v5:
>  - Added /proc/sys/fs/dio_short_writes (default 1) to guard older applications
>    which expect write(fd, buf, count) returns either count or error.
> 
> Changes since v6:
>  - Corrected documentation
>  - Re-ordered patch
> 
> diff --git a/Documentation/sysctl/fs.txt b/Documentation/sysctl/fs.txt
> index 6c00c1e2743f..21582f675985 100644
> --- a/Documentation/sysctl/fs.txt
> +++ b/Documentation/sysctl/fs.txt
> @@ -22,6 +22,7 @@ Currently, these files are in /proc/sys/fs:
>  - aio-max-nr
>  - aio-nr
>  - dentry-state
> +- dio_short_writes
>  - dquot-max
>  - dquot-nr
>  - file-max
> @@ -76,6 +77,19 @@ dcache isn't pruned yet.
>  
>  ==============================================================
>  
> +dio_short_writes:
> +
> +In case Direct I/O encounters a transient error, it returns
> +the error code, even if it has performed part of the write.
> +This flag, if on (default), will return the number of bytes written
> +so far, as the write(2) semantics are. However, some older applications
> +still consider a direct write as an error if all of the I/O
> +submitted is not complete. I.e. write(file, count, buf) != count.
> +This option can be disabled on systems in order to support
> +existing applications which do not expect short writes.
> +
> +==============================================================
> +
>  dquot-max & dquot-nr:
>  
>  The file dquot-max shows the maximum number of cached disk
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 4a181fcb5175..49d94360bb51 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -409,7 +409,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 3aafb3343a65..9bd15be64c25 100644
> --- a/fs/direct-io.c
> +++ b/fs/direct-io.c
> @@ -151,6 +151,7 @@ struct dio {
>  } ____cacheline_aligned_in_smp;
>  
>  static struct kmem_cache *dio_cache __read_mostly;
> +unsigned int sysctl_dio_short_writes = 1;
>  
>  /*
>   * How many pages are in the queue?
> @@ -262,7 +263,7 @@ static ssize_t dio_complete(struct dio *dio, ssize_t ret, unsigned int flags)
>  		ret = dio->page_errors;
>  	if (ret == 0)
>  		ret = dio->io_error;
> -	if (ret == 0)
> +	if (!sysctl_dio_short_writes && (ret == 0))
>  		ret = transferred;
>  
>  	if (dio->end_io) {
> @@ -310,7 +311,9 @@ static ssize_t dio_complete(struct dio *dio, ssize_t ret, unsigned int flags)
>  	}
>  
>  	kmem_cache_free(dio_cache, dio);
> -	return ret;
> +	if (!sysctl_dio_short_writes)
> +		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 47d29ccffaef..a8d6908dc0de 100644
> --- a/fs/iomap.c
> +++ b/fs/iomap.c
> @@ -716,23 +716,24 @@ static ssize_t iomap_dio_complete(struct iomap_dio *dio)
>  	struct kiocb *iocb = dio->iocb;
>  	struct inode *inode = file_inode(iocb->ki_filp);
>  	loff_t offset = iocb->ki_pos;
> -	ssize_t ret;
> +	ssize_t err;
> +	ssize_t transferred = dio->size;

I'm sorry to bring this up again, but there's something not quite right
with this.  Every time iomap_dio_actor create a bio, it increments
dio->size by bio->bi_iter.bi_size before calling submit_bio.  dio->size is
the 'partial' size returned to the caller if there's an error, which
means that if we write a single 2MB bio and it fails, we still get a
partial result of 2MB, not zero.

Analysis of generic/250 bears this out:

total 40960
drwxr-xr-x 2 root root       19 Mar  7 15:59 .
drwxr-xr-x 3 root root       22 Mar  7 15:59 ..
-rw------- 1 root root 41943040 Mar  7 15:59 file2
Filesystem type is: 58465342
File size of /opt/test-250/file2 is 41943040 (10240 blocks of 4096
ytes)
 ext:     logical_offset:        physical_offset: length:   expected:
lags:
   0:        0..     511:         24..       535:    512:
   1:      512..    2047:        536..      2071:   1536: unwritten
   2:     2048..    2048:       2072..      2072:      1:
   3:     2049..    6249:       2073..      6273:   4201: unwritten
   4:     6250..   10239:       6416..     10405:   3990:       6274:
last,unwritten,eof
/opt/test-250/file2: 2 extents found
0000000  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00
         \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
*
0032768  69  69  69  69  69  69  69  69  69  69  69  69  69  69  69  69
          i   i   i   i   i   i   i   i   i   i   i   i   i   i   i   i
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Note that we wrote 0x69 to the disk prior to mkfs so that if any
unwritten extents were incorrectly converted to real extents we'd detect
it immediately.  This is evidence that we're exposing stale disk
contents.

*
2097152  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00
         \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
*
8388608  63  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00
          c  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
8388624  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00
         \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
*
41943040

I think there's a more serious problem here too.  Let's say userspace
asks for a 4MB dio write and the dio write itself splits into four 1MB
write bios.  bio 0, 2, and 3 return quickly, but bio 1 fails slowly,
which means we successfully wrote 0-1M and 2M-3M, but since we can't
communicate a vector back to userspace the best we can do is return
1048576.

I think this is going to need better state tracking of exactly /what/
succeeded before we can return partial writes to userspace.  This could
be as simple as recording the iomap offset with each bio issued and
reducing dio->size to min(dio->size, bio->iomap->offset) if
bio->bi_status is set in iomap_dio_bio_end_io.

--D

>  
>  	if (dio->end_io) {
> -		ret = dio->end_io(iocb,
> -				dio->error ? dio->error : dio->size,
> -				dio->flags);
> +		err = dio->end_io(iocb,
> +				  (transferred && sysctl_dio_short_writes) ?
> +						transferred : dio->error,
> +				  dio->flags);
>  	} else {
> -		ret = dio->error;
> +		err = dio->error;
>  	}
>  
> -	if (likely(!ret)) {
> -		ret = dio->size;
> +	if (likely(transferred)) {
>  		/* check for short read */
> -		if (offset + ret > dio->i_size &&
> +		if (offset + transferred > dio->i_size &&
>  		    !(dio->flags & IOMAP_DIO_WRITE))
> -			ret = dio->i_size - offset;
> -		iocb->ki_pos += ret;
> +			transferred = dio->i_size - offset;
> +		iocb->ki_pos += transferred;
>  	}
>  
>  	/*
> @@ -759,7 +760,7 @@ static ssize_t iomap_dio_complete(struct iomap_dio *dio)
>  	inode_dio_end(file_inode(iocb->ki_filp));
>  	kfree(dio);
>  
> -	return ret;
> +	return (transferred && sysctl_dio_short_writes) ? transferred : err;
>  }
>  
>  static void iomap_dio_complete_work(struct work_struct *work)
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 511fbaabf624..a25652e5ae1b 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1469,6 +1469,7 @@ static inline void i_gid_write(struct inode *inode, gid_t gid)
>  }
>  
>  extern struct timespec current_time(struct inode *inode);
> +extern unsigned int sysctl_dio_short_writes;
>  
>  /*
>   * Snapshotting support.
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 557d46728577..362a9c3156f1 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -1844,6 +1844,15 @@ static struct ctl_table fs_table[] = {
>  		.proc_handler	= proc_dointvec_minmax,
>  		.extra1		= &one,
>  	},
> +	{
> +		.procname	= "dio_short_writes",
> +		.data		= &sysctl_dio_short_writes,
> +		.maxlen		= sizeof(int),
> +		.mode		= 0600,
> +		.proc_handler	= proc_dointvec_minmax,
> +		.extra1		= &zero,
> +		.extra2		= &one,
> +	},
>  	{ }
>  };
>  
> -- 
> 2.16.1
> 

Re: [PATCH v7 2/2] Return bytes transferred for partial direct I/O

[Goldwyn Rodrigues]

On 03/07/2018 06:53 PM, Darrick J. Wong wrote:
> On Thu, Feb 08, 2018 at 12:59:48PM -0600, Goldwyn Rodrigues wrote:
>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>
>> 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.
>>
>> Added a sysctl entry: dio_short_writes which is on by default. This is
>> to support applications which expect either and error or the bytes submitted
>> as a return value for the write calls.
>>
>> This fixes fstest generic/472.
>>
>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
>> ---
>>  Documentation/sysctl/fs.txt | 14 ++++++++++++++
>>  fs/block_dev.c              |  2 +-
>>  fs/direct-io.c              |  7 +++++--
>>  fs/iomap.c                  | 23 ++++++++++++-----------
>>  include/linux/fs.h          |  1 +
>>  kernel/sysctl.c             |  9 +++++++++
>>  6 files changed, 42 insertions(+), 14 deletions(-)
>>
>> 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. Fixed.
>>
>> Changes since v3:
>>  - [hch] initialize transferred with dio->size and use transferred instead
>>    of dio->size.
>>
>> Changes since v4:
>>  - Refreshed to v4.14
>>
>> Changes since v5:
>>  - Added /proc/sys/fs/dio_short_writes (default 1) to guard older applications
>>    which expect write(fd, buf, count) returns either count or error.
>>
>> Changes since v6:
>>  - Corrected documentation
>>  - Re-ordered patch
>>
>> diff --git a/Documentation/sysctl/fs.txt b/Documentation/sysctl/fs.txt
>> index 6c00c1e2743f..21582f675985 100644
>> --- a/Documentation/sysctl/fs.txt
>> +++ b/Documentation/sysctl/fs.txt
>> @@ -22,6 +22,7 @@ Currently, these files are in /proc/sys/fs:
>>  - aio-max-nr
>>  - aio-nr
>>  - dentry-state
>> +- dio_short_writes
>>  - dquot-max
>>  - dquot-nr
>>  - file-max
>> @@ -76,6 +77,19 @@ dcache isn't pruned yet.
>>  
>>  ==============================================================
>>  
>> +dio_short_writes:
>> +
>> +In case Direct I/O encounters a transient error, it returns
>> +the error code, even if it has performed part of the write.
>> +This flag, if on (default), will return the number of bytes written
>> +so far, as the write(2) semantics are. However, some older applications
>> +still consider a direct write as an error if all of the I/O
>> +submitted is not complete. I.e. write(file, count, buf) != count.
>> +This option can be disabled on systems in order to support
>> +existing applications which do not expect short writes.
>> +
>> +==============================================================
>> +
>>  dquot-max & dquot-nr:
>>  
>>  The file dquot-max shows the maximum number of cached disk
>> diff --git a/fs/block_dev.c b/fs/block_dev.c
>> index 4a181fcb5175..49d94360bb51 100644
>> --- a/fs/block_dev.c
>> +++ b/fs/block_dev.c
>> @@ -409,7 +409,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 3aafb3343a65..9bd15be64c25 100644
>> --- a/fs/direct-io.c
>> +++ b/fs/direct-io.c
>> @@ -151,6 +151,7 @@ struct dio {
>>  } ____cacheline_aligned_in_smp;
>>  
>>  static struct kmem_cache *dio_cache __read_mostly;
>> +unsigned int sysctl_dio_short_writes = 1;
>>  
>>  /*
>>   * How many pages are in the queue?
>> @@ -262,7 +263,7 @@ static ssize_t dio_complete(struct dio *dio, ssize_t ret, unsigned int flags)
>>  		ret = dio->page_errors;
>>  	if (ret == 0)
>>  		ret = dio->io_error;
>> -	if (ret == 0)
>> +	if (!sysctl_dio_short_writes && (ret == 0))
>>  		ret = transferred;
>>  
>>  	if (dio->end_io) {
>> @@ -310,7 +311,9 @@ static ssize_t dio_complete(struct dio *dio, ssize_t ret, unsigned int flags)
>>  	}
>>  
>>  	kmem_cache_free(dio_cache, dio);
>> -	return ret;
>> +	if (!sysctl_dio_short_writes)
>> +		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 47d29ccffaef..a8d6908dc0de 100644
>> --- a/fs/iomap.c
>> +++ b/fs/iomap.c
>> @@ -716,23 +716,24 @@ static ssize_t iomap_dio_complete(struct iomap_dio *dio)
>>  	struct kiocb *iocb = dio->iocb;
>>  	struct inode *inode = file_inode(iocb->ki_filp);
>>  	loff_t offset = iocb->ki_pos;
>> -	ssize_t ret;
>> +	ssize_t err;
>> +	ssize_t transferred = dio->size;
> 
> I'm sorry to bring this up again, but there's something not quite right
> with this.  Every time iomap_dio_actor create a bio, it increments
> dio->size by bio->bi_iter.bi_size before calling submit_bio.  dio->size is
> the 'partial' size returned to the caller if there's an error, which
> means that if we write a single 2MB bio and it fails, we still get a
> partial result of 2MB, not zero.
> 
> Analysis of generic/250 bears this out:
> 
> total 40960
> drwxr-xr-x 2 root root       19 Mar  7 15:59 .
> drwxr-xr-x 3 root root       22 Mar  7 15:59 ..
> -rw------- 1 root root 41943040 Mar  7 15:59 file2
> Filesystem type is: 58465342
> File size of /opt/test-250/file2 is 41943040 (10240 blocks of 4096
> ytes)
>  ext:     logical_offset:        physical_offset: length:   expected:
> lags:
>    0:        0..     511:         24..       535:    512:
>    1:      512..    2047:        536..      2071:   1536: unwritten
>    2:     2048..    2048:       2072..      2072:      1:
>    3:     2049..    6249:       2073..      6273:   4201: unwritten
>    4:     6250..   10239:       6416..     10405:   3990:       6274:
> last,unwritten,eof
> /opt/test-250/file2: 2 extents found
> 0000000  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00
>          \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
> *
> 0032768  69  69  69  69  69  69  69  69  69  69  69  69  69  69  69  69
>           i   i   i   i   i   i   i   i   i   i   i   i   i   i   i   i
>          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Note that we wrote 0x69 to the disk prior to mkfs so that if any
> unwritten extents were incorrectly converted to real extents we'd detect
> it immediately.  This is evidence that we're exposing stale disk
> contents.
> 
> *
> 2097152  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00
>          \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
> *
> 8388608  63  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00
>           c  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
> 8388624  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00
>          \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
> *
> 41943040
> 
> I think there's a more serious problem here too.  Let's say userspace
> asks for a 4MB dio write and the dio write itself splits into four 1MB
> write bios.  bio 0, 2, and 3 return quickly, but bio 1 fails slowly,
> which means we successfully wrote 0-1M and 2M-3M, but since we can't
> communicate a vector back to userspace the best we can do is return
> 1048576.

Yes, this is a known problem and the only solution I have been told is
to document it. But it the light of what you have expressed earlier, yes
this patch does not make sense. An error in the direct I/O means that
the data in the range may be inconsistent/garbage.

> 
> I think this is going to need better state tracking of exactly /what/
> succeeded before we can return partial writes to userspace.  This could
> be as simple as recording the iomap offset with each bio issued and
> reducing dio->size to min(dio->size, bio->iomap->offset) if
> bio->bi_status is set in iomap_dio_bio_end_io.
> 
What about the rest of the data? Should the user assume that the *rest*
of the data (count - ret) is inconsistent in case of a short write?

-- 
Goldwyn

Re: [PATCH v7 2/2] Return bytes transferred for partial direct I/O

[Darrick J. Wong]

On Thu, Mar 08, 2018 at 09:35:48AM -0600, Goldwyn Rodrigues wrote:
> 
> 
> On 03/07/2018 06:53 PM, Darrick J. Wong wrote:
> > On Thu, Feb 08, 2018 at 12:59:48PM -0600, Goldwyn Rodrigues wrote:
> >> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> >>
> >> 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.
> >>
> >> Added a sysctl entry: dio_short_writes which is on by default. This is
> >> to support applications which expect either and error or the bytes submitted
> >> as a return value for the write calls.
> >>
> >> This fixes fstest generic/472.
> >>
> >> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> >> ---
> >>  Documentation/sysctl/fs.txt | 14 ++++++++++++++
> >>  fs/block_dev.c              |  2 +-
> >>  fs/direct-io.c              |  7 +++++--
> >>  fs/iomap.c                  | 23 ++++++++++++-----------
> >>  include/linux/fs.h          |  1 +
> >>  kernel/sysctl.c             |  9 +++++++++
> >>  6 files changed, 42 insertions(+), 14 deletions(-)
> >>
> >> 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. Fixed.
> >>
> >> Changes since v3:
> >>  - [hch] initialize transferred with dio->size and use transferred instead
> >>    of dio->size.
> >>
> >> Changes since v4:
> >>  - Refreshed to v4.14
> >>
> >> Changes since v5:
> >>  - Added /proc/sys/fs/dio_short_writes (default 1) to guard older applications
> >>    which expect write(fd, buf, count) returns either count or error.
> >>
> >> Changes since v6:
> >>  - Corrected documentation
> >>  - Re-ordered patch
> >>
> >> diff --git a/Documentation/sysctl/fs.txt b/Documentation/sysctl/fs.txt
> >> index 6c00c1e2743f..21582f675985 100644
> >> --- a/Documentation/sysctl/fs.txt
> >> +++ b/Documentation/sysctl/fs.txt
> >> @@ -22,6 +22,7 @@ Currently, these files are in /proc/sys/fs:
> >>  - aio-max-nr
> >>  - aio-nr
> >>  - dentry-state
> >> +- dio_short_writes
> >>  - dquot-max
> >>  - dquot-nr
> >>  - file-max
> >> @@ -76,6 +77,19 @@ dcache isn't pruned yet.
> >>  
> >>  ==============================================================
> >>  
> >> +dio_short_writes:
> >> +
> >> +In case Direct I/O encounters a transient error, it returns
> >> +the error code, even if it has performed part of the write.
> >> +This flag, if on (default), will return the number of bytes written
> >> +so far, as the write(2) semantics are. However, some older applications
> >> +still consider a direct write as an error if all of the I/O
> >> +submitted is not complete. I.e. write(file, count, buf) != count.
> >> +This option can be disabled on systems in order to support
> >> +existing applications which do not expect short writes.
> >> +
> >> +==============================================================
> >> +
> >>  dquot-max & dquot-nr:
> >>  
> >>  The file dquot-max shows the maximum number of cached disk
> >> diff --git a/fs/block_dev.c b/fs/block_dev.c
> >> index 4a181fcb5175..49d94360bb51 100644
> >> --- a/fs/block_dev.c
> >> +++ b/fs/block_dev.c
> >> @@ -409,7 +409,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 3aafb3343a65..9bd15be64c25 100644
> >> --- a/fs/direct-io.c
> >> +++ b/fs/direct-io.c
> >> @@ -151,6 +151,7 @@ struct dio {
> >>  } ____cacheline_aligned_in_smp;
> >>  
> >>  static struct kmem_cache *dio_cache __read_mostly;
> >> +unsigned int sysctl_dio_short_writes = 1;
> >>  
> >>  /*
> >>   * How many pages are in the queue?
> >> @@ -262,7 +263,7 @@ static ssize_t dio_complete(struct dio *dio, ssize_t ret, unsigned int flags)
> >>  		ret = dio->page_errors;
> >>  	if (ret == 0)
> >>  		ret = dio->io_error;
> >> -	if (ret == 0)
> >> +	if (!sysctl_dio_short_writes && (ret == 0))
> >>  		ret = transferred;
> >>  
> >>  	if (dio->end_io) {
> >> @@ -310,7 +311,9 @@ static ssize_t dio_complete(struct dio *dio, ssize_t ret, unsigned int flags)
> >>  	}
> >>  
> >>  	kmem_cache_free(dio_cache, dio);
> >> -	return ret;
> >> +	if (!sysctl_dio_short_writes)
> >> +		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 47d29ccffaef..a8d6908dc0de 100644
> >> --- a/fs/iomap.c
> >> +++ b/fs/iomap.c
> >> @@ -716,23 +716,24 @@ static ssize_t iomap_dio_complete(struct iomap_dio *dio)
> >>  	struct kiocb *iocb = dio->iocb;
> >>  	struct inode *inode = file_inode(iocb->ki_filp);
> >>  	loff_t offset = iocb->ki_pos;
> >> -	ssize_t ret;
> >> +	ssize_t err;
> >> +	ssize_t transferred = dio->size;
> > 
> > I'm sorry to bring this up again, but there's something not quite right
> > with this.  Every time iomap_dio_actor create a bio, it increments
> > dio->size by bio->bi_iter.bi_size before calling submit_bio.  dio->size is
> > the 'partial' size returned to the caller if there's an error, which
> > means that if we write a single 2MB bio and it fails, we still get a
> > partial result of 2MB, not zero.
> > 
> > Analysis of generic/250 bears this out:
> > 
> > total 40960
> > drwxr-xr-x 2 root root       19 Mar  7 15:59 .
> > drwxr-xr-x 3 root root       22 Mar  7 15:59 ..
> > -rw------- 1 root root 41943040 Mar  7 15:59 file2
> > Filesystem type is: 58465342
> > File size of /opt/test-250/file2 is 41943040 (10240 blocks of 4096
> > ytes)
> >  ext:     logical_offset:        physical_offset: length:   expected:
> > lags:
> >    0:        0..     511:         24..       535:    512:
> >    1:      512..    2047:        536..      2071:   1536: unwritten
> >    2:     2048..    2048:       2072..      2072:      1:
> >    3:     2049..    6249:       2073..      6273:   4201: unwritten
> >    4:     6250..   10239:       6416..     10405:   3990:       6274:
> > last,unwritten,eof
> > /opt/test-250/file2: 2 extents found
> > 0000000  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00
> >          \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
> > *
> > 0032768  69  69  69  69  69  69  69  69  69  69  69  69  69  69  69  69
> >           i   i   i   i   i   i   i   i   i   i   i   i   i   i   i   i
> >          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > Note that we wrote 0x69 to the disk prior to mkfs so that if any
> > unwritten extents were incorrectly converted to real extents we'd detect
> > it immediately.  This is evidence that we're exposing stale disk
> > contents.
> > 
> > *
> > 2097152  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00
> >          \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
> > *
> > 8388608  63  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00
> >           c  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
> > 8388624  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00  00
> >          \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0  \0
> > *
> > 41943040
> > 
> > I think there's a more serious problem here too.  Let's say userspace
> > asks for a 4MB dio write and the dio write itself splits into four 1MB
> > write bios.  bio 0, 2, and 3 return quickly, but bio 1 fails slowly,
> > which means we successfully wrote 0-1M and 2M-3M, but since we can't
> > communicate a vector back to userspace the best we can do is return
> > 1048576.
> 
> Yes, this is a known problem and the only solution I have been told is
> to document it.

<nod> I think I'm comfortable returning the offset of the first failing
io as a lower bound for "bytes written".  Presumably the file pointer
will be advanced by that amount and the operation retried from there, if
the program cares to do so.

> But it the light of what you have expressed earlier, yes
> this patch does not make sense. An error in the direct I/O means that
> the data in the range may be inconsistent/garbage.
> 
> > 
> > I think this is going to need better state tracking of exactly /what/
> > succeeded before we can return partial writes to userspace.  This could
> > be as simple as recording the iomap offset with each bio issued and
> > reducing dio->size to min(dio->size, bio->iomap->offset) if
> > bio->bi_status is set in iomap_dio_bio_end_io.
> > 
> What about the rest of the data? Should the user assume that the *rest*
> of the data (count - ret) is inconsistent in case of a short write?

Well, a narrow reading of the spec would be that we only made a
statement about the range (offset, offset + ret), so who cares what
happened past that? :P  But it does break the model that "write says it
wrote X bytes, so everything past X remains as it was before"... but
this is directio where everything is upside down. :P

--D

> 
> -- 
> Goldwyn

Re: [PATCH v7 2/2] Return bytes transferred for partial direct I/O

[Theodore Y. Ts'o]

On Thu, Feb 08, 2018 at 12:59:48PM -0600, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> 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.
> 
> Added a sysctl entry: dio_short_writes which is on by default. This is
> to support applications which expect either and error or the bytes submitted
> as a return value for the write calls.
> 
> This fixes fstest generic/472.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

Hi,

I was wondering if you could give an update regarding what's up with
this patch?

There doesn't seem to be any movement on this patch in a while, and so
I still have xfstests generic/472 suppressed in {kvm,gce}-xfstests.

Thanks,

						- Ted

Re: [PATCH v7 2/2] Return bytes transferred for partial direct I/O

[Goldwyn Rodrigues]

On 05/19/2018 08:29 PM, Theodore Y. Ts'o wrote:
> On Thu, Feb 08, 2018 at 12:59:48PM -0600, Goldwyn Rodrigues wrote:
>> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>>
>> 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.
>>
>> Added a sysctl entry: dio_short_writes which is on by default. This is
>> to support applications which expect either and error or the bytes submitted
>> as a return value for the write calls.
>>
>> This fixes fstest generic/472.
>>
>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Hi,
> 
> I was wondering if you could give an update regarding what's up with
> this patch?
> 
> There doesn't seem to be any movement on this patch in a while, and so
> I still have xfstests generic/472 suppressed in {kvm,gce}-xfstests.
> 

>From earlier discussions, In between errors of a direct I/O cannot be
handled correctly and may need a lot of tracking that it is not worth
performing.

It would be better to drop this test case and add in the documentation
that a direct I/O error could mean that the write() may or may not have
occurred and underlying data may be inconsistent. I am proposing:

diff --git a/man2/write.2 b/man2/write.2
index f8a94f3ff..86f655e26 100644
--- a/man2/write.2
+++ b/man2/write.2
@@ -274,6 +274,14 @@ On Linux,
 returning the number of bytes actually transferred.
 .\" commit e28cc71572da38a5a12c1cfe4d7032017adccf69
 (This is true on both 32-bit and 64-bit systems.)
+.PP
+While performing
+.BR write()
+using direct I/O, an error returned does not mean the
+entire write has failed. Partial data may be written
+and the file offset to length on which the
+.BR write()
+was attempted should be considered inconsistent.
 .SH BUGS
 According to POSIX.1-2008/SUSv4 Section XSI 2.9.7
 ("Thread Interactions with Regular File Operations"):


-- 
Goldwyn

Re: [PATCH v7 1/2] xfs: remove assert to check bytes returned

[Darrick J. Wong]

On Thu, Feb 08, 2018 at 12:59:47PM -0600, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Since we can return less than count in case of partial direct
> writes, remove the ASSERT.
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D