| Message ID | 20220309094323.2082884-1-yang.yang29@zte.com.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/psi: remove PSI annotations from submit_bio | expand |
On Wed, Mar 09, 2022 at 09:43:24AM +0000, cgel.zte@gmail.com wrote: > From: Yang Yang <yang.yang29@zte.com.cn> > > psi tracks the time spent submitting the IO of refaulting pages[1]. > But after we tracks refault stalls from swap_readpage[2][3], there > is no need to do so anymore. Since swap_readpage already includes > IO submitting time. > > [1] commit b8e24a9300b0 ("block: annotate refault stalls from IO submission") > [2] commit 937790699be9 ("mm/page_io.c: annotate refault stalls from swap_readpage") > [3] commit 2b413a1a728f ("mm: page_io: fix psi memory pressure error on cold swapins") > > Signed-off-by: Yang Yang <yang.yang29@zte.com.cn> > Reviewed-by: Ran Xiaokai <ran.xiaokai@zte.com.cn> It's still needed by file cache refaults! NAK
On Wed, Mar 09, 2022 at 02:18:19PM -0500, Johannes Weiner wrote: > On Wed, Mar 09, 2022 at 09:43:24AM +0000, cgel.zte@gmail.com wrote: > > From: Yang Yang <yang.yang29@zte.com.cn> > > > > psi tracks the time spent submitting the IO of refaulting pages[1]. > > But after we tracks refault stalls from swap_readpage[2][3], there > > is no need to do so anymore. Since swap_readpage already includes > > IO submitting time. > > > > [1] commit b8e24a9300b0 ("block: annotate refault stalls from IO submission") > > [2] commit 937790699be9 ("mm/page_io.c: annotate refault stalls from swap_readpage") > > [3] commit 2b413a1a728f ("mm: page_io: fix psi memory pressure error on cold swapins") > > > > Signed-off-by: Yang Yang <yang.yang29@zte.com.cn> > > Reviewed-by: Ran Xiaokai <ran.xiaokai@zte.com.cn> > > It's still needed by file cache refaults! Can we get proper annotations for those please? These bio-level hooks are horrible and a maintainance nightmware.
On Wed, Mar 09, 2022 at 10:46:52PM -0800, Christoph Hellwig wrote: > On Wed, Mar 09, 2022 at 02:18:19PM -0500, Johannes Weiner wrote: > > On Wed, Mar 09, 2022 at 09:43:24AM +0000, cgel.zte@gmail.com wrote: > > > From: Yang Yang <yang.yang29@zte.com.cn> > > > > > > psi tracks the time spent submitting the IO of refaulting pages[1]. > > > But after we tracks refault stalls from swap_readpage[2][3], there > > > is no need to do so anymore. Since swap_readpage already includes > > > IO submitting time. > > > > > > [1] commit b8e24a9300b0 ("block: annotate refault stalls from IO submission") > > > [2] commit 937790699be9 ("mm/page_io.c: annotate refault stalls from swap_readpage") > > > [3] commit 2b413a1a728f ("mm: page_io: fix psi memory pressure error on cold swapins") > > > > > > Signed-off-by: Yang Yang <yang.yang29@zte.com.cn> > > > Reviewed-by: Ran Xiaokai <ran.xiaokai@zte.com.cn> > > > > It's still needed by file cache refaults! > > Can we get proper annotations for those please? These bio-level hooks are > horrible and a maintainance nightmware. The first version did that, but it was sprawling and not well-received: https://lkml.org/lkml/2019/7/22/1261
On Thu, Mar 10, 2022 at 11:16:41AM -0500, Johannes Weiner wrote: > The first version did that, but it was sprawling and not well-received: > > https://lkml.org/lkml/2019/7/22/1261 Well, Dave's comments are spot on. Except that we replaced it with something even more horrible and not something sensible as he suggested.
On Thu, Mar 10, 2022 at 08:18:31AM -0800, Christoph Hellwig wrote: > On Thu, Mar 10, 2022 at 11:16:41AM -0500, Johannes Weiner wrote: > > The first version did that, but it was sprawling and not well-received: > > > > https://lkml.org/lkml/2019/7/22/1261 > > Well, Dave's comments are spot on. Except that we replaced it with > something even more horrible and not something sensible as he suggested. Confused. I changed it the way Dave suggested, to which he replied "this is much cleaner and easier to maintain". Are we reading different threads? Care to elaborate?
On Wed, Mar 09, 2022 at 02:18:19PM -0500, Johannes Weiner wrote: > On Wed, Mar 09, 2022 at 09:43:24AM +0000, cgel.zte@gmail.com wrote: > > From: Yang Yang <yang.yang29@zte.com.cn> > > > > psi tracks the time spent submitting the IO of refaulting pages[1]. > > But after we tracks refault stalls from swap_readpage[2][3], there > > is no need to do so anymore. Since swap_readpage already includes > > IO submitting time. > > > > [1] commit b8e24a9300b0 ("block: annotate refault stalls from IO submission") > > [2] commit 937790699be9 ("mm/page_io.c: annotate refault stalls from swap_readpage") > > [3] commit 2b413a1a728f ("mm: page_io: fix psi memory pressure error on cold swapins") > > > > Signed-off-by: Yang Yang <yang.yang29@zte.com.cn> > > Reviewed-by: Ran Xiaokai <ran.xiaokai@zte.com.cn> > > It's still needed by file cache refaults! > > NAK What about submit_bio() only counts file cache refaults? This will reduce redundant calling of psi_memstall_enter() for swap_readpage().
diff --git a/block/bio.c b/block/bio.c index 3c57b3ba727d..efbbeed348e3 100644 --- a/block/bio.c +++ b/block/bio.c @@ -1034,9 +1034,6 @@ void __bio_add_page(struct bio *bio, struct page *page, bio->bi_iter.bi_size += len; bio->bi_vcnt++; - - if (!bio_flagged(bio, BIO_WORKINGSET) && unlikely(PageWorkingset(page))) - bio_set_flag(bio, BIO_WORKINGSET); } EXPORT_SYMBOL_GPL(__bio_add_page); @@ -1252,9 +1249,6 @@ static int __bio_iov_append_get_pages(struct bio *bio, struct iov_iter *iter) * fit into the bio, or are requested in @iter, whatever is smaller. If * MM encounters an error pinning the requested pages, it stops. Error * is returned only if 0 pages could be pinned. - * - * It's intended for direct IO, so doesn't do PSI tracking, the caller is - * responsible for setting BIO_WORKINGSET if necessary. */ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter) { @@ -1273,8 +1267,6 @@ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter) ret = __bio_iov_iter_get_pages(bio, iter); } while (!ret && iov_iter_count(iter) && !bio_full(bio, 0)); - /* don't account direct I/O as memory stall */ - bio_clear_flag(bio, BIO_WORKINGSET); return bio->bi_vcnt ? 0 : ret; } EXPORT_SYMBOL_GPL(bio_iov_iter_get_pages); diff --git a/block/blk-core.c b/block/blk-core.c index ddac62aebc55..faf7d950a4d5 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -37,7 +37,6 @@ #include <linux/t10-pi.h> #include <linux/debugfs.h> #include <linux/bpf.h> -#include <linux/psi.h> #include <linux/part_stat.h> #include <linux/sched/sysctl.h> #include <linux/blk-crypto.h> @@ -911,22 +910,6 @@ void submit_bio(struct bio *bio) } } - /* - * If we're reading data that is part of the userspace workingset, count - * submission time as memory stall. When the device is congested, or - * the submitting cgroup IO-throttled, submission can be a significant - * part of overall IO time. - */ - if (unlikely(bio_op(bio) == REQ_OP_READ && - bio_flagged(bio, BIO_WORKINGSET))) { - unsigned long pflags; - - psi_memstall_enter(&pflags); - submit_bio_noacct(bio); - psi_memstall_leave(&pflags); - return; - } - submit_bio_noacct(bio); } EXPORT_SYMBOL(submit_bio); diff --git a/fs/direct-io.c b/fs/direct-io.c index aef06e607b40..5cac8c8869c5 100644 --- a/fs/direct-io.c +++ b/fs/direct-io.c @@ -419,8 +419,6 @@ static inline void dio_bio_submit(struct dio *dio, struct dio_submit *sdio) unsigned long flags; bio->bi_private = dio; - /* don't account direct I/O as memory stall */ - bio_clear_flag(bio, BIO_WORKINGSET); spin_lock_irqsave(&dio->bio_lock, flags); dio->refcount++; diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h index 0c7a9a1f06c8..ab50c59b02ce 100644 --- a/include/linux/blk_types.h +++ b/include/linux/blk_types.h @@ -314,7 +314,6 @@ enum { BIO_NO_PAGE_REF, /* don't put release vec pages */ BIO_CLONED, /* doesn't own data */ BIO_BOUNCED, /* bio is a bounce bio */ - BIO_WORKINGSET, /* contains userspace workingset pages */ BIO_QUIET, /* Make BIO Quiet */ BIO_CHAIN, /* chained bio, ->bi_remaining in effect */ BIO_REFFED, /* bio has elevated ->bi_cnt */