Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1377088 > unrolled thread

[PATCH v3 1/2] ext4: Pass in DIO_SKIP_DIO_COUNT flag if inode_dio_begin() called

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 1/2] ext4: Pass in DIO_SKIP_DIO_COUNT flag if inode_dio_begin() called Waiman Long <Waiman.Long@hpe.com> - 2016-04-12 20:20 +0200
    Re: [PATCH v3 1/2] ext4: Pass in DIO_SKIP_DIO_COUNT flag if  inode_dio_begin() called Dave Chinner <david@fromorbit.com> - 2016-04-14 05:20 +0200
      Re: [PATCH v3 1/2] ext4: Pass in DIO_SKIP_DIO_COUNT flag if inode_dio_begin()  called Waiman Long <waiman.long@hpe.com> - 2016-04-14 18:30 +0200
        Re: [PATCH v3 1/2] ext4: Pass in DIO_SKIP_DIO_COUNT flag if  inode_dio_begin() called Dave Chinner <david@fromorbit.com> - 2016-04-15 10:20 +0200

#1377088 — [PATCH v3 1/2] ext4: Pass in DIO_SKIP_DIO_COUNT flag if inode_dio_begin() called

When performing direct I/O, the current ext4 code does
not pass in the DIO_SKIP_DIO_COUNT flag to dax_do_io() or
__blockdev_direct_IO() when inode_dio_begin() has, in fact, been
called. This causes dax_do_io()/__blockdev_direct_IO() to invoke
inode_dio_begin()/inode_dio_end() internally.  This doubling of
inode_dio_begin()/inode_dio_end() calls are wasteful.

This patch removes the extra internal inode_dio_begin()/inode_dio_end()
calls when those calls are being issued by the caller directly. For
really fast storage systems like NVDIMM, the removal of the extra
inode_dio_begin()/inode_dio_end() can give a meaningful boost to
I/O performance.

On a 4-socket Haswell-EX system (72 cores) running 4.6-rc1 kernel,
fio with 38 threads doing parallel I/O on two shared files on an
NVDIMM with DAX gave the following aggregrate bandwidth with and
without the patch:

  Test          W/O patch       With patch      % change
  ----          ---------       ----------      --------
  Read-only      8688MB/s       10173MB/s        +17.1%
  Read-write     2687MB/s        2830MB/s         +5.3%

Signed-off-by: Waiman Long <Waiman.Long@hpe.com>
---
 fs/ext4/indirect.c |   10 ++++++++--
 fs/ext4/inode.c    |   12 +++++++++---
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c
index 3027fa6..4304be6 100644
--- a/fs/ext4/indirect.c
+++ b/fs/ext4/indirect.c
@@ -706,14 +706,20 @@ retry:
 			inode_dio_end(inode);
 			goto locked;
 		}
+		/*
+		 * Need to pass in DIO_SKIP_DIO_COUNT to prevent
+		 * duplicated inode_dio_begin/inode_dio_end sequence.
+		 */
 		if (IS_DAX(inode))
 			ret = dax_do_io(iocb, inode, iter, offset,
-					ext4_dio_get_block, NULL, 0);
+					ext4_dio_get_block, NULL,
+					DIO_SKIP_DIO_COUNT);
 		else
 			ret = __blockdev_direct_IO(iocb, inode,
 						   inode->i_sb->s_bdev, iter,
 						   offset, ext4_dio_get_block,
-						   NULL, NULL, 0);
+						   NULL, NULL,
+						   DIO_SKIP_DIO_COUNT);
 		inode_dio_end(inode);
 	} else {
 locked:
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index dab84a2..779aa33 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3358,9 +3358,15 @@ static ssize_t ext4_ext_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
 	 * Make all waiters for direct IO properly wait also for extent
 	 * conversion. This also disallows race between truncate() and
 	 * overwrite DIO as i_dio_count needs to be incremented under i_mutex.
+	 *
+	 * Both dax_do_io() and __blockdev_direct_IO() will unnecessarily
+	 * call inode_dio_begin()/inode_dio_end() again if the
+	 * DIO_SKIP_DIO_COUNT flag is not set.
 	 */
-	if (iov_iter_rw(iter) == WRITE)
+	if (iov_iter_rw(iter) == WRITE) {
+		dio_flags = DIO_SKIP_DIO_COUNT;
 		inode_dio_begin(inode);
+	}
 
 	/* If we do a overwrite dio, i_mutex locking can be released */
 	overwrite = *((int *)iocb->private);
@@ -3393,10 +3399,10 @@ static ssize_t ext4_ext_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
 		get_block_func = ext4_dio_get_block_overwrite;
 	else if (is_sync_kiocb(iocb)) {
 		get_block_func = ext4_dio_get_block_unwritten_sync;
-		dio_flags = DIO_LOCKING;
+		dio_flags |= DIO_LOCKING;
 	} else {
 		get_block_func = ext4_dio_get_block_unwritten_async;
-		dio_flags = DIO_LOCKING;
+		dio_flags |= DIO_LOCKING;
 	}
 #ifdef CONFIG_EXT4_FS_ENCRYPTION
 	BUG_ON(ext4_encrypted_inode(inode) && S_ISREG(inode->i_mode));
-- 
1.7.1

[toc] | [next] | [standalone]

#1378436 — Re: [PATCH v3 1/2] ext4: Pass in DIO_SKIP_DIO_COUNT flag if inode_dio_begin() called

On Tue, Apr 12, 2016 at 02:12:54PM -0400, Waiman Long wrote:
> When performing direct I/O, the current ext4 code does
> not pass in the DIO_SKIP_DIO_COUNT flag to dax_do_io() or
> __blockdev_direct_IO() when inode_dio_begin() has, in fact, been
> called. This causes dax_do_io()/__blockdev_direct_IO() to invoke
> inode_dio_begin()/inode_dio_end() internally.  This doubling of
> inode_dio_begin()/inode_dio_end() calls are wasteful.
> 
> This patch removes the extra internal inode_dio_begin()/inode_dio_end()
> calls when those calls are being issued by the caller directly. For
> really fast storage systems like NVDIMM, the removal of the extra
> inode_dio_begin()/inode_dio_end() can give a meaningful boost to
> I/O performance.

Doesn't this break truncate IO serialisation?

i.e. it appears to me that the ext4 use of inode_dio_begin()/
inode_dio_end() does not cover AIO, where the IO is still in flight
when submission returns. i.e. the inode_dio_end() call
needs to be in IO completion, not in the submitter context. The only
reason it doesn't break right now is that the duplicate accounting
in the DIO code is correct w.r.t. AIO. Hence bypassing the DIO
accounting will cause AIO writes to race with truncate.

Same AIO vs truncate problem occurs with the indirect read case you
modified to skip the direct IO layer accounting.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[toc] | [prev] | [next] | [standalone]

#1379071 — Re: [PATCH v3 1/2] ext4: Pass in DIO_SKIP_DIO_COUNT flag if inode_dio_begin() called

On 04/13/2016 11:16 PM, Dave Chinner wrote:
> On Tue, Apr 12, 2016 at 02:12:54PM -0400, Waiman Long wrote:
>> When performing direct I/O, the current ext4 code does
>> not pass in the DIO_SKIP_DIO_COUNT flag to dax_do_io() or
>> __blockdev_direct_IO() when inode_dio_begin() has, in fact, been
>> called. This causes dax_do_io()/__blockdev_direct_IO() to invoke
>> inode_dio_begin()/inode_dio_end() internally.  This doubling of
>> inode_dio_begin()/inode_dio_end() calls are wasteful.
>>
>> This patch removes the extra internal inode_dio_begin()/inode_dio_end()
>> calls when those calls are being issued by the caller directly. For
>> really fast storage systems like NVDIMM, the removal of the extra
>> inode_dio_begin()/inode_dio_end() can give a meaningful boost to
>> I/O performance.
> Doesn't this break truncate IO serialisation?
>
> i.e. it appears to me that the ext4 use of inode_dio_begin()/
> inode_dio_end() does not cover AIO, where the IO is still in flight
> when submission returns. i.e. the inode_dio_end() call
> needs to be in IO completion, not in the submitter context. The only
> reason it doesn't break right now is that the duplicate accounting
> in the DIO code is correct w.r.t. AIO. Hence bypassing the DIO
> accounting will cause AIO writes to race with truncate.
>
> Same AIO vs truncate problem occurs with the indirect read case you
> modified to skip the direct IO layer accounting.

I don't quite understand how the duplicate accounting is correct wrt 
AIO. Both the direct and indirect paths are something like:

     inode_dio_begin()
     ...
         inode_dio_begin()
         ...
         inode_dio_end()
     ...
     inode_dio_end()

What the patch does is to eliminate the innermost inode_dio_begin/end 
pair. Unless there is a difference between a dio count of 2 vs. 1, I 
can't see how the code correctness differ with and without my patch.

Cheers,
Longman

[toc] | [prev] | [next] | [standalone]

#1379538 — Re: [PATCH v3 1/2] ext4: Pass in DIO_SKIP_DIO_COUNT flag if inode_dio_begin() called

On Thu, Apr 14, 2016 at 12:21:13PM -0400, Waiman Long wrote:
> On 04/13/2016 11:16 PM, Dave Chinner wrote:
> >On Tue, Apr 12, 2016 at 02:12:54PM -0400, Waiman Long wrote:
> >>When performing direct I/O, the current ext4 code does
> >>not pass in the DIO_SKIP_DIO_COUNT flag to dax_do_io() or
> >>__blockdev_direct_IO() when inode_dio_begin() has, in fact, been
> >>called. This causes dax_do_io()/__blockdev_direct_IO() to invoke
> >>inode_dio_begin()/inode_dio_end() internally.  This doubling of
> >>inode_dio_begin()/inode_dio_end() calls are wasteful.
> >>
> >>This patch removes the extra internal inode_dio_begin()/inode_dio_end()
> >>calls when those calls are being issued by the caller directly. For
> >>really fast storage systems like NVDIMM, the removal of the extra
> >>inode_dio_begin()/inode_dio_end() can give a meaningful boost to
> >>I/O performance.
> >Doesn't this break truncate IO serialisation?
> >
> >i.e. it appears to me that the ext4 use of inode_dio_begin()/
> >inode_dio_end() does not cover AIO, where the IO is still in flight
> >when submission returns. i.e. the inode_dio_end() call
> >needs to be in IO completion, not in the submitter context. The only
> >reason it doesn't break right now is that the duplicate accounting
> >in the DIO code is correct w.r.t. AIO. Hence bypassing the DIO
> >accounting will cause AIO writes to race with truncate.
> >
> >Same AIO vs truncate problem occurs with the indirect read case you
> >modified to skip the direct IO layer accounting.
> 
> I don't quite understand how the duplicate accounting is correct wrt
> AIO. Both the direct and indirect paths are something like:
> 
>     inode_dio_begin()
>     ...
>         inode_dio_begin()
>         ...
>         inode_dio_end()
>     ...
>     inode_dio_end()

With AIO:

	inode_dio_begin()
	...
		inode_dio_begin()
		<submit IO, no wait>
	...
	inode_dio_end()
<ext4 returns to userspace with AIO+DIO in progress>

<some time later DIO completes>
	        dio_complete
		  inode_dio_end()

IOWs, the ext4 accounting is broken w.r.t. AIO, where IO submission
does not wait for IO completion before returning.

> What the patch does is to eliminate the innermost
> inode_dio_begin/end pair.

Yes, and with that change inode_dio_wait() no longer waits for
AIO+DIO writes on ext4, hence breaking truncate IO barrier
requirements of inode_dio_wait().

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web