* [PATCH v5] DAX: enable iostat for read/write
@ 2017-01-12 18:38 ` Toshi Kani
0 siblings, 0 replies; 14+ messages in thread
From: Toshi Kani @ 2017-01-12 18:38 UTC (permalink / raw)
To: akpm, dan.j.williams
Cc: david, viro, ross.zwisler, linux-nvdimm, linux-fsdevel,
linux-kernel, Toshi Kani, Joe Perches
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>
---
v5:
- Add a flag in case 'start' is 0 after 'jiffies' rolls over.
(Dan Williams)
- Fix a signed/unsigned conversion. (Joe Perches)
---
fs/dax.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/fs/dax.c b/fs/dax.c
index 5c74f60..a3e406a 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) {
+ sector_t sec = iov_iter_count(iter) >> 9;
+
+ start = jiffies;
+ generic_start_io_acct(iov_iter_rw(iter),
+ min_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;
}
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v5] DAX: enable iostat for read/write
@ 2017-01-12 18:38 ` Toshi Kani
0 siblings, 0 replies; 14+ messages in thread
From: Toshi Kani @ 2017-01-12 18:38 UTC (permalink / raw)
To: akpm, dan.j.williams
Cc: david, viro, ross.zwisler, linux-nvdimm, linux-fsdevel,
linux-kernel, Toshi Kani, Joe Perches
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>
---
v5:
- Add a flag in case 'start' is 0 after 'jiffies' rolls over.
(Dan Williams)
- Fix a signed/unsigned conversion. (Joe Perches)
---
fs/dax.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/fs/dax.c b/fs/dax.c
index 5c74f60..a3e406a 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) {
+ sector_t sec = iov_iter_count(iter) >> 9;
+
+ start = jiffies;
+ generic_start_io_acct(iov_iter_rw(iter),
+ min_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;
}
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v5] DAX: enable iostat for read/write
2017-01-12 18:38 ` Toshi Kani
(?)
@ 2017-01-12 18:02 ` Joe Perches
-1 siblings, 0 replies; 14+ messages in thread
From: Joe Perches @ 2017-01-12 18:02 UTC (permalink / raw)
To: Toshi Kani, akpm, dan.j.williams
Cc: linux-nvdimm, david, linux-kernel, viro, linux-fsdevel
On Thu, 2017-01-12 at 11:38 -0700, Toshi Kani 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.
[]
> diff --git 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) {
> + sector_t sec = iov_iter_count(iter) >> 9;
> +
> + start = jiffies;
> + generic_start_io_acct(iov_iter_rw(iter),
> + min_t(unsigned long, 1, sec),
I believe I mislead you with a thinko.
Your original code was
(!sec) ? 1 : sec
and I suggested incorrectly using min_t
It should of course be max_t. Sorry.
Also, as sec is now sector_t (u64), perhaps this
unsigned long cast is incorrect.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v5] DAX: enable iostat for read/write
@ 2017-01-12 18:02 ` Joe Perches
0 siblings, 0 replies; 14+ messages in thread
From: Joe Perches @ 2017-01-12 18:02 UTC (permalink / raw)
To: Toshi Kani, akpm, dan.j.williams
Cc: david, viro, ross.zwisler, linux-nvdimm, linux-fsdevel, linux-kernel
On Thu, 2017-01-12 at 11:38 -0700, Toshi Kani 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.
[]
> diff --git 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) {
> + sector_t sec = iov_iter_count(iter) >> 9;
> +
> + start = jiffies;
> + generic_start_io_acct(iov_iter_rw(iter),
> + min_t(unsigned long, 1, sec),
I believe I mislead you with a thinko.
Your original code was
(!sec) ? 1 : sec
and I suggested incorrectly using min_t
It should of course be max_t. Sorry.
Also, as sec is now sector_t (u64), perhaps this
unsigned long cast is incorrect.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v5] DAX: enable iostat for read/write
@ 2017-01-12 18:02 ` Joe Perches
0 siblings, 0 replies; 14+ messages in thread
From: Joe Perches @ 2017-01-12 18:02 UTC (permalink / raw)
To: Toshi Kani, akpm, dan.j.williams
Cc: david, viro, ross.zwisler, linux-nvdimm, linux-fsdevel, linux-kernel
On Thu, 2017-01-12 at 11:38 -0700, Toshi Kani 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.
[]
> diff --git 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) {
> + sector_t sec = iov_iter_count(iter) >> 9;
> +
> + start = jiffies;
> + generic_start_io_acct(iov_iter_rw(iter),
> + min_t(unsigned long, 1, sec),
I believe I mislead you with a thinko.
Your original code was
(!sec) ? 1 : sec
and I suggested incorrectly using min_t
It should of course be max_t. Sorry.
Also, as sec is now sector_t (u64), perhaps this
unsigned long cast is incorrect.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v5] DAX: enable iostat for read/write
2017-01-12 18:02 ` Joe Perches
(?)
@ 2017-01-12 18:26 ` Kani, Toshimitsu
-1 siblings, 0 replies; 14+ messages in thread
From: Kani, Toshimitsu @ 2017-01-12 18:26 UTC (permalink / raw)
To: joe, dan.j.williams, akpm
Cc: linux-nvdimm, david, linux-kernel, viro, linux-fsdevel
On Thu, 2017-01-12 at 10:02 -0800, Joe Perches wrote:
> On Thu, 2017-01-12 at 11:38 -0700, Toshi Kani 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.
>
> []
> > diff --git 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) {
> > + sector_t sec = iov_iter_count(iter) >> 9;
> > +
> > + start = jiffies;
> > + generic_start_io_acct(iov_iter_rw(iter),
> > + min_t(unsigned long, 1,
> > sec),
>
> I believe I mislead you with a thinko.
>
> Your original code was
> (!sec) ? 1 : sec
> and I suggested incorrectly using min_t
>
> It should of course be max_t. Sorry.
My bad. I should have caught it.
> Also, as sec is now sector_t (u64), perhaps this
> unsigned long cast is incorrect.
I see. Since iov_iter_count() returns a size_t value, I will use
'size_t' for 'sec' as you originally suggested.
Thanks,
-Toshi
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v5] DAX: enable iostat for read/write
@ 2017-01-12 18:26 ` Kani, Toshimitsu
0 siblings, 0 replies; 14+ messages in thread
From: Kani, Toshimitsu @ 2017-01-12 18:26 UTC (permalink / raw)
To: joe, dan.j.williams, akpm
Cc: viro, david, linux-nvdimm, ross.zwisler, linux-fsdevel, linux-kernel
On Thu, 2017-01-12 at 10:02 -0800, Joe Perches wrote:
> On Thu, 2017-01-12 at 11:38 -0700, Toshi Kani 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.
>
> []
> > diff --git 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) {
> > + sector_t sec = iov_iter_count(iter) >> 9;
> > +
> > + start = jiffies;
> > + generic_start_io_acct(iov_iter_rw(iter),
> > + min_t(unsigned long, 1,
> > sec),
>
> I believe I mislead you with a thinko.
>
> Your original code was
> (!sec) ? 1 : sec
> and I suggested incorrectly using min_t
>
> It should of course be max_t. Sorry.
My bad. I should have caught it.
> Also, as sec is now sector_t (u64), perhaps this
> unsigned long cast is incorrect.
I see. Since iov_iter_count() returns a size_t value, I will use
'size_t' for 'sec' as you originally suggested.
Thanks,
-Toshi
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v5] DAX: enable iostat for read/write
@ 2017-01-12 18:26 ` Kani, Toshimitsu
0 siblings, 0 replies; 14+ messages in thread
From: Kani, Toshimitsu @ 2017-01-12 18:26 UTC (permalink / raw)
To: joe, dan.j.williams, akpm
Cc: viro, david, linux-nvdimm@lists.01.org, ross.zwisler,
linux-fsdevel, linux-kernel
On Thu, 2017-01-12 at 10:02 -0800, Joe Perches wrote:
> On Thu, 2017-01-12 at 11:38 -0700, Toshi Kani 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.
>
> []
> > diff --git 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) {
> > + sector_t sec = iov_iter_count(iter) >> 9;
> > +
> > + start = jiffies;
> > + generic_start_io_acct(iov_iter_rw(iter),
> > + min_t(unsigned long, 1,
> > sec),
>
> I believe I mislead you with a thinko.
>
> Your original code was
> (!sec) ? 1 : sec
> and I suggested incorrectly using min_t
>
> It should of course be max_t. Sorry.
My bad. I should have caught it.
> Also, as sec is now sector_t (u64), perhaps this
> unsigned long cast is incorrect.
I see. Since iov_iter_count() returns a size_t value, I will use
'size_t' for 'sec' as you originally suggested.
Thanks,
-Toshi
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5] DAX: enable iostat for read/write
2017-01-12 18:38 ` Toshi Kani
(?)
@ 2017-01-25 11:19 ` Christoph Hellwig
-1 siblings, 0 replies; 14+ messages in thread
From: Christoph Hellwig @ 2017-01-25 11:19 UTC (permalink / raw)
To: Toshi Kani
Cc: linux-nvdimm, david, linux-kernel, linux-fsdevel, viro,
Joe Perches, akpm
On Thu, Jan 12, 2017 at 11:38:48AM -0700, Toshi Kani 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.
NAK. DAX I/O should not be accounted for block device statistics.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5] DAX: enable iostat for read/write
@ 2017-01-25 11:19 ` Christoph Hellwig
0 siblings, 0 replies; 14+ messages in thread
From: Christoph Hellwig @ 2017-01-25 11:19 UTC (permalink / raw)
To: Toshi Kani
Cc: akpm, dan.j.williams, linux-nvdimm, david, linux-kernel,
Joe Perches, viro, linux-fsdevel
On Thu, Jan 12, 2017 at 11:38:48AM -0700, Toshi Kani 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.
NAK. DAX I/O should not be accounted for block device statistics.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5] DAX: enable iostat for read/write
@ 2017-01-25 11:19 ` Christoph Hellwig
0 siblings, 0 replies; 14+ messages in thread
From: Christoph Hellwig @ 2017-01-25 11:19 UTC (permalink / raw)
To: Toshi Kani
Cc: akpm, dan.j.williams, linux-nvdimm, david, linux-kernel,
Joe Perches, viro, linux-fsdevel
On Thu, Jan 12, 2017 at 11:38:48AM -0700, Toshi Kani 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.
NAK. DAX I/O should not be accounted for block device statistics.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5] DAX: enable iostat for read/write
2017-01-25 11:19 ` Christoph Hellwig
(?)
(?)
@ 2017-01-25 15:15 ` Jeff Moyer
2017-01-25 15:55 ` Kani, Toshimitsu
-1 siblings, 1 reply; 14+ messages in thread
From: Jeff Moyer @ 2017-01-25 15:15 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Toshi Kani, akpm, dan.j.williams, linux-nvdimm, david,
linux-kernel, Joe Perches, viro, linux-fsdevel
Christoph Hellwig <hch@infradead.org> writes:
> On Thu, Jan 12, 2017 at 11:38:48AM -0700, Toshi Kani 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.
>
> NAK. DAX I/O should not be accounted for block device statistics.
Agreed, this is a layering violation.
Cheers,
Jeff
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5] DAX: enable iostat for read/write
2017-01-25 15:15 ` Jeff Moyer
@ 2017-01-25 15:55 ` Kani, Toshimitsu
0 siblings, 0 replies; 14+ messages in thread
From: Kani, Toshimitsu @ 2017-01-25 15:55 UTC (permalink / raw)
To: hch, jmoyer
Cc: linux-kernel, linux-nvdimm, viro, dan.j.williams, akpm, joe,
linux-fsdevel, david
On Wed, 2017-01-25 at 10:15 -0500, Jeff Moyer wrote:
> Christoph Hellwig <hch@infradead.org> writes:
>
> > On Thu, Jan 12, 2017 at 11:38:48AM -0700, Toshi Kani 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.
> >
> > NAK. DAX I/O should not be accounted for block device statistics.
>
> Agreed, this is a layering violation.
I will check to see if it can fit on top of Dan's patch-set.
Thanks,
-Toshi
^ permalink raw reply [flat|nested] 14+ messages in thread