Re: [PATCH v6] DAX: enable iostat for read/write

Re: [PATCH v6] DAX: enable iostat for read/write

[Andrew Morton]

On Fri, 13 Jan 2017 16:34:18 -0700 Toshi Kani <toshi.kani@hpe.com> wrote:

> DAX IO path does not support iostat, but its metadata IO path does.
> Therefore, iostat shows metadata IO statistics only, which has been
> confusing to users.
> 
> Add iostat support to the DAX read/write path.
> 
> Note, iostat still does not support the DAX mmap path as it allows
> user applications to access directly.
> 
> ...
>
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -1058,12 +1058,24 @@ dax_iomap_rw(struct kiocb *iocb, struct iov_iter *iter,
>  {
>  	struct address_space *mapping = iocb->ki_filp->f_mapping;
>  	struct inode *inode = mapping->host;
> +	struct gendisk *disk = inode->i_sb->s_bdev->bd_disk;
>  	loff_t pos = iocb->ki_pos, ret = 0, done = 0;
>  	unsigned flags = 0;
> +	unsigned long start = 0;
> +	int do_acct = blk_queue_io_stat(disk->queue);

(The poorly named) blk_queue_io_stat() actually returns a bool.  This
is well concealed because blk_queue_io_stat() is unnecessarily
implemented as a macro (why oh why).

--- a/fs/dax.c~dax-enable-iostat-for-read-write-fix
+++ a/fs/dax.c
@@ -1085,7 +1085,7 @@ dax_iomap_rw(struct kiocb *iocb, struct
 	loff_t pos = iocb->ki_pos, ret = 0, done = 0;
 	unsigned flags = 0;
 	unsigned long start = 0;
-	int do_acct = blk_queue_io_stat(disk->queue);
+	bool do_acct = blk_queue_io_stat(disk->queue);
 
 	if (iov_iter_rw(iter) == WRITE)
 		flags |= IOMAP_WRITE;
_

Re: [PATCH v6] DAX: enable iostat for read/write

[Kani, Toshimitsu]

On Fri, 2017-01-13 at 14:54 -0800, Andrew Morton wrote:
> On Fri, 13 Jan 2017 16:34:18 -0700 Toshi Kani <toshi.kani@hpe.com>
> wrote:
> 
> > DAX IO path does not support iostat, but its metadata IO path does.
> > Therefore, iostat shows metadata IO statistics only, which has been
> > confusing to users.
> > 
> > Add iostat support to the DAX read/write path.
> > 
> > Note, iostat still does not support the DAX mmap path as it allows
> > user applications to access directly.
> > 
> > ...
> > 
> > --- a/fs/dax.c
> > +++ b/fs/dax.c
> > @@ -1058,12 +1058,24 @@ dax_iomap_rw(struct kiocb *iocb, struct
> > iov_iter *iter,
> >  {
> >  	struct address_space *mapping = iocb->ki_filp->f_mapping;
> >  	struct inode *inode = mapping->host;
> > +	struct gendisk *disk = inode->i_sb->s_bdev->bd_disk;
> >  	loff_t pos = iocb->ki_pos, ret = 0, done = 0;
> >  	unsigned flags = 0;
> > +	unsigned long start = 0;
> > +	int do_acct = blk_queue_io_stat(disk->queue);
> 
> (The poorly named) blk_queue_io_stat() actually returns a bool.  This
> is well concealed because blk_queue_io_stat() is unnecessarily
> implemented as a macro (why oh why).

It was unclear to me what type I needed to use.  test_bit() is 'bool'
in arch/x86/include/asm/bitops.h but is 'int' in  include/asm-
generic/bitops/non-atomic.h.  So, I used 'int' for safe... 

Thanks,
-Toshi


> 
> --- a/fs/dax.c~dax-enable-iostat-for-read-write-fix
> +++ a/fs/dax.c
> @@ -1085,7 +1085,7 @@ dax_iomap_rw(struct kiocb *iocb, struct
>  	loff_t pos = iocb->ki_pos, ret = 0, done = 0;
>  	unsigned flags = 0;
>  	unsigned long start = 0;
> -	int do_acct = blk_queue_io_stat(disk->queue);
> +	bool do_acct = blk_queue_io_stat(disk->queue);
>  
>  	if (iov_iter_rw(iter) == WRITE)
>  		flags |= IOMAP_WRITE;
> _
> 

Re: [PATCH v6] DAX: enable iostat for read/write

[Andrew Morton]

On Fri, 13 Jan 2017 23:09:58 +0000 "Kani, Toshimitsu" <toshi.kani@hpe.com> wrote:

> On Fri, 2017-01-13 at 14:54 -0800, Andrew Morton wrote:
> > On Fri, 13 Jan 2017 16:34:18 -0700 Toshi Kani <toshi.kani@hpe.com>
> > wrote:
> > 
> > > DAX IO path does not support iostat, but its metadata IO path does.
> > > Therefore, iostat shows metadata IO statistics only, which has been
> > > confusing to users.
> > > 
> > > Add iostat support to the DAX read/write path.
> > > 
> > > Note, iostat still does not support the DAX mmap path as it allows
> > > user applications to access directly.
> > > 
> > > ...
> > > 
> > > --- a/fs/dax.c
> > > +++ b/fs/dax.c
> > > @@ -1058,12 +1058,24 @@ dax_iomap_rw(struct kiocb *iocb, struct
> > > iov_iter *iter,
> > > __{
> > > __	struct address_space *mapping = iocb->ki_filp->f_mapping;
> > > __	struct inode *inode = mapping->host;
> > > +	struct gendisk *disk = inode->i_sb->s_bdev->bd_disk;
> > > __	loff_t pos = iocb->ki_pos, ret = 0, done = 0;
> > > __	unsigned flags = 0;
> > > +	unsigned long start = 0;
> > > +	int do_acct = blk_queue_io_stat(disk->queue);
> > 
> > (The poorly named) blk_queue_io_stat() actually returns a bool.____This
> > is well concealed because blk_queue_io_stat() is unnecessarily
> > implemented as a macro (why oh why).
> 
> It was unclear to me what type I needed to use.  test_bit() is 'bool'
> in arch/x86/include/asm/bitops.h but is 'int' in  include/asm-
> generic/bitops/non-atomic.h.  So, I used 'int' for safe... 

Yes, x86 does appear to be an outlier.  Mess.

[PATCH v6] DAX: enable iostat for read/write

[Toshi Kani]

DAX IO path does not support iostat, but its metadata IO path does.
Therefore, iostat shows metadata IO statistics only, which has been
confusing to users.

Add iostat support to the DAX read/write path.

Note, iostat still does not support the DAX mmap path as it allows
user applications to access directly.

Signed-off-by: Toshi Kani <toshi.kani@hpe.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Dave Chinner <david@fromorbit.com>
Cc: Ross Zwisler <ross.zwisler@linux.intel.com>
Cc: Joe Perches <joe@perches.com>
---
v6:
 - Change min_t() to max_t(), and sector_t to size_t. (Joe Perches)
---
 fs/dax.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/fs/dax.c b/fs/dax.c
index 5c74f60..ac04f21 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -1058,12 +1058,24 @@ dax_iomap_rw(struct kiocb *iocb, struct iov_iter *iter,
 {
 	struct address_space *mapping = iocb->ki_filp->f_mapping;
 	struct inode *inode = mapping->host;
+	struct gendisk *disk = inode->i_sb->s_bdev->bd_disk;
 	loff_t pos = iocb->ki_pos, ret = 0, done = 0;
 	unsigned flags = 0;
+	unsigned long start = 0;
+	int do_acct = blk_queue_io_stat(disk->queue);
 
 	if (iov_iter_rw(iter) == WRITE)
 		flags |= IOMAP_WRITE;
 
+	if (do_acct) {
+		size_t sec = iov_iter_count(iter) >> 9;
+
+		start = jiffies;
+		generic_start_io_acct(iov_iter_rw(iter),
+				      max_t(unsigned long, 1, sec),
+				      &disk->part0);
+	}
+
 	while (iov_iter_count(iter)) {
 		ret = iomap_apply(inode, pos, iov_iter_count(iter), flags, ops,
 				iter, dax_iomap_actor);
@@ -1073,6 +1085,9 @@ dax_iomap_rw(struct kiocb *iocb, struct iov_iter *iter,
 		done += ret;
 	}
 
+	if (do_acct)
+		generic_end_io_acct(iov_iter_rw(iter), &disk->part0, start);
+
 	iocb->ki_pos += done;
 	return done ? done : ret;
 }