| Message ID | 1d3cf86668e44b3a3d35b5dbe759a086a157e434.1607976425.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | no-copy bvec | expand |
On Tue, Dec 15, 2020 at 12:20:23AM +0000, Pavel Begunkov wrote: > As reported, we must not do pressure stall information accounting for > direct IO, because otherwise it tells that it's thrashing a page when > actually doing IO on hot data. > > Apparently, bio_iov_iter_get_pages() is used only by paths doing direct > IO, so just make it avoid setting BIO_WORKINGSET, it also saves us CPU > cycles on doing that. For fs/direct-io.c just clear the flag before > submit_bio(), it's not of much concern performance-wise. > > Reported-by: Christoph Hellwig <hch@infradead.org> > Suggested-by: Christoph Hellwig <hch@infradead.org> > Suggested-by: Johannes Weiner <hannes@cmpxchg.org> > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > --- > block/bio.c | 25 ++++++++++++++++--------- > fs/direct-io.c | 2 ++ > 2 files changed, 18 insertions(+), 9 deletions(-) ..... > @@ -1099,6 +1103,9 @@ 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 also doesn't set BIO_WORKINGSET, so is intended for direct IO. If used > + * otherwise the caller is responsible to do that to keep PSI happy. > */ > int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter) > { > diff --git a/fs/direct-io.c b/fs/direct-io.c > index d53fa92a1ab6..914a7f600ecd 100644 > --- a/fs/direct-io.c > +++ b/fs/direct-io.c > @@ -426,6 +426,8 @@ static inline void dio_bio_submit(struct dio *dio, struct dio_submit *sdio) > unsigned long flags; > > bio->bi_private = dio; > + /* PSI is only for paging IO */ > + bio_clear_flag(bio, BIO_WORKINGSET); Why only do this for the old direct IO path? Why isn't this necessary for the iomap DIO path? Cheers, Dave.
On 15/12/2020 00:56, Dave Chinner wrote: > On Tue, Dec 15, 2020 at 12:20:23AM +0000, Pavel Begunkov wrote: >> As reported, we must not do pressure stall information accounting for >> direct IO, because otherwise it tells that it's thrashing a page when >> actually doing IO on hot data. >> >> Apparently, bio_iov_iter_get_pages() is used only by paths doing direct >> IO, so just make it avoid setting BIO_WORKINGSET, it also saves us CPU >> cycles on doing that. For fs/direct-io.c just clear the flag before >> submit_bio(), it's not of much concern performance-wise. >> >> Reported-by: Christoph Hellwig <hch@infradead.org> >> Suggested-by: Christoph Hellwig <hch@infradead.org> >> Suggested-by: Johannes Weiner <hannes@cmpxchg.org> >> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >> --- >> block/bio.c | 25 ++++++++++++++++--------- >> fs/direct-io.c | 2 ++ >> 2 files changed, 18 insertions(+), 9 deletions(-) > ..... >> @@ -1099,6 +1103,9 @@ 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 also doesn't set BIO_WORKINGSET, so is intended for direct IO. If used >> + * otherwise the caller is responsible to do that to keep PSI happy. >> */ >> int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter) >> { >> diff --git a/fs/direct-io.c b/fs/direct-io.c >> index d53fa92a1ab6..914a7f600ecd 100644 >> --- a/fs/direct-io.c >> +++ b/fs/direct-io.c >> @@ -426,6 +426,8 @@ static inline void dio_bio_submit(struct dio *dio, struct dio_submit *sdio) >> unsigned long flags; >> >> bio->bi_private = dio; >> + /* PSI is only for paging IO */ >> + bio_clear_flag(bio, BIO_WORKINGSET); > > Why only do this for the old direct IO path? Why isn't this > necessary for the iomap DIO path? It's in the description. In short, block and iomap dio use bio_iov_iter_get_pages(), which with this patch doesn't use [__]bio_add_page() and so doesn't set the flag.
On Tue, Dec 15, 2020 at 01:03:45AM +0000, Pavel Begunkov wrote: > On 15/12/2020 00:56, Dave Chinner wrote: > > On Tue, Dec 15, 2020 at 12:20:23AM +0000, Pavel Begunkov wrote: > >> As reported, we must not do pressure stall information accounting for > >> direct IO, because otherwise it tells that it's thrashing a page when > >> actually doing IO on hot data. > >> > >> Apparently, bio_iov_iter_get_pages() is used only by paths doing direct > >> IO, so just make it avoid setting BIO_WORKINGSET, it also saves us CPU > >> cycles on doing that. For fs/direct-io.c just clear the flag before > >> submit_bio(), it's not of much concern performance-wise. > >> > >> Reported-by: Christoph Hellwig <hch@infradead.org> > >> Suggested-by: Christoph Hellwig <hch@infradead.org> > >> Suggested-by: Johannes Weiner <hannes@cmpxchg.org> > >> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > >> --- > >> block/bio.c | 25 ++++++++++++++++--------- > >> fs/direct-io.c | 2 ++ > >> 2 files changed, 18 insertions(+), 9 deletions(-) > > ..... > >> @@ -1099,6 +1103,9 @@ 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 also doesn't set BIO_WORKINGSET, so is intended for direct IO. If used > >> + * otherwise the caller is responsible to do that to keep PSI happy. > >> */ > >> int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter) > >> { > >> diff --git a/fs/direct-io.c b/fs/direct-io.c > >> index d53fa92a1ab6..914a7f600ecd 100644 > >> --- a/fs/direct-io.c > >> +++ b/fs/direct-io.c > >> @@ -426,6 +426,8 @@ static inline void dio_bio_submit(struct dio *dio, struct dio_submit *sdio) > >> unsigned long flags; > >> > >> bio->bi_private = dio; > >> + /* PSI is only for paging IO */ > >> + bio_clear_flag(bio, BIO_WORKINGSET); > > > > Why only do this for the old direct IO path? Why isn't this > > necessary for the iomap DIO path? > > It's in the description. In short, block and iomap dio use > bio_iov_iter_get_pages(), which with this patch doesn't use > [__]bio_add_page() and so doesn't set the flag. That is not obvious to someone not intimately familiar with the patchset you are working on. You described -what- the code is doing, not -why- the flag needs to be cleared here. "Direct IO does not operate on the current working set of pages managed by the kernel, so it should not be accounted as IO to the pressure stall tracking infrastructure. Only direct IO paths use bio_iov_iter_get_pages() to build bios, so to avoid PSI tracking of direct IO don't flag the bio with BIO_WORKINGSET in this function. fs/direct-io.c uses <some other function> to build the bio we are going to submit and so still flags the bio with BIO_WORKINGSET. Rather than convert it to use bio_iov_iter_get_pages() to avoid flagging the bio, we simply clear the BIO_WORKINGSET flag before submitting the bio." Cheers, Dave.
On 15/12/2020 01:33, Dave Chinner wrote: > On Tue, Dec 15, 2020 at 01:03:45AM +0000, Pavel Begunkov wrote: >> On 15/12/2020 00:56, Dave Chinner wrote: >>> On Tue, Dec 15, 2020 at 12:20:23AM +0000, Pavel Begunkov wrote: >>>> As reported, we must not do pressure stall information accounting for >>>> direct IO, because otherwise it tells that it's thrashing a page when >>>> actually doing IO on hot data. >>>> >>>> Apparently, bio_iov_iter_get_pages() is used only by paths doing direct >>>> IO, so just make it avoid setting BIO_WORKINGSET, it also saves us CPU >>>> cycles on doing that. For fs/direct-io.c just clear the flag before >>>> submit_bio(), it's not of much concern performance-wise. >>>> >>>> Reported-by: Christoph Hellwig <hch@infradead.org> >>>> Suggested-by: Christoph Hellwig <hch@infradead.org> >>>> Suggested-by: Johannes Weiner <hannes@cmpxchg.org> >>>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >>>> --- >>>> block/bio.c | 25 ++++++++++++++++--------- >>>> fs/direct-io.c | 2 ++ >>>> 2 files changed, 18 insertions(+), 9 deletions(-) >>> ..... >>>> @@ -1099,6 +1103,9 @@ 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 also doesn't set BIO_WORKINGSET, so is intended for direct IO. If used >>>> + * otherwise the caller is responsible to do that to keep PSI happy. >>>> */ >>>> int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter) >>>> { >>>> diff --git a/fs/direct-io.c b/fs/direct-io.c >>>> index d53fa92a1ab6..914a7f600ecd 100644 >>>> --- a/fs/direct-io.c >>>> +++ b/fs/direct-io.c >>>> @@ -426,6 +426,8 @@ static inline void dio_bio_submit(struct dio *dio, struct dio_submit *sdio) >>>> unsigned long flags; >>>> >>>> bio->bi_private = dio; >>>> + /* PSI is only for paging IO */ >>>> + bio_clear_flag(bio, BIO_WORKINGSET); >>> >>> Why only do this for the old direct IO path? Why isn't this >>> necessary for the iomap DIO path? >> >> It's in the description. In short, block and iomap dio use >> bio_iov_iter_get_pages(), which with this patch doesn't use >> [__]bio_add_page() and so doesn't set the flag. > > That is not obvious to someone not intimately familiar with the > patchset you are working on. You described -what- the code is doing, > not -why- the flag needs to be cleared here. It's missing the link between BIO_WORKINGSET and PSI, but otherwise it describe both, what it does and how. I'll reword it for you next iteration. > > "Direct IO does not operate on the current working set of pages > managed by the kernel, so it should not be accounted as IO to the > pressure stall tracking infrastructure. Only direct IO paths use > bio_iov_iter_get_pages() to build bios, so to avoid PSI tracking of > direct IO don't flag the bio with BIO_WORKINGSET in this function. > > fs/direct-io.c uses <some other function> to build the bio we > are going to submit and so still flags the bio with BIO_WORKINGSET. > Rather than convert it to use bio_iov_iter_get_pages() to avoid > flagging the bio, we simply clear the BIO_WORKINGSET flag before > submitting the bio." > > Cheers, > > Dave. >
I think the commit log could use some rewording with less "as reported" and "apparently". IMHO it is more obvious to just clear the flag in __bio_iov_bvec_add_pages in this patch as well, and just go straight to the rewrite later instead of this intermediate stage.
diff --git a/block/bio.c b/block/bio.c index 4a8f77bb3956..3192358c411f 100644 --- a/block/bio.c +++ b/block/bio.c @@ -963,18 +963,22 @@ EXPORT_SYMBOL_GPL(bio_release_pages); static int __bio_iov_bvec_add_pages(struct bio *bio, struct iov_iter *iter) { const struct bio_vec *bv = iter->bvec; - unsigned int len; - size_t size; + struct page *page = bv->bv_page; + bool same_page = false; + unsigned int off, len; if (WARN_ON_ONCE(iter->iov_offset > bv->bv_len)) return -EINVAL; len = min_t(size_t, bv->bv_len - iter->iov_offset, iter->count); - size = bio_add_page(bio, bv->bv_page, len, - bv->bv_offset + iter->iov_offset); - if (unlikely(size != len)) - return -EINVAL; - iov_iter_advance(iter, size); + off = bv->bv_offset + iter->iov_offset; + + if (!__bio_try_merge_page(bio, page, len, off, &same_page)) { + if (bio_full(bio, len)) + return -EINVAL; + bio_add_page_noaccount(bio, page, len, off); + } + iov_iter_advance(iter, len); return 0; } @@ -1023,8 +1027,8 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter) put_page(page); } else { if (WARN_ON_ONCE(bio_full(bio, len))) - return -EINVAL; - __bio_add_page(bio, page, len, offset); + return -EINVAL; + bio_add_page_noaccount(bio, page, len, offset); } offset = 0; } @@ -1099,6 +1103,9 @@ 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 also doesn't set BIO_WORKINGSET, so is intended for direct IO. If used + * otherwise the caller is responsible to do that to keep PSI happy. */ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter) { diff --git a/fs/direct-io.c b/fs/direct-io.c index d53fa92a1ab6..914a7f600ecd 100644 --- a/fs/direct-io.c +++ b/fs/direct-io.c @@ -426,6 +426,8 @@ static inline void dio_bio_submit(struct dio *dio, struct dio_submit *sdio) unsigned long flags; bio->bi_private = dio; + /* PSI is only for paging IO */ + bio_clear_flag(bio, BIO_WORKINGSET); spin_lock_irqsave(&dio->bio_lock, flags); dio->refcount++;
As reported, we must not do pressure stall information accounting for direct IO, because otherwise it tells that it's thrashing a page when actually doing IO on hot data. Apparently, bio_iov_iter_get_pages() is used only by paths doing direct IO, so just make it avoid setting BIO_WORKINGSET, it also saves us CPU cycles on doing that. For fs/direct-io.c just clear the flag before submit_bio(), it's not of much concern performance-wise. Reported-by: Christoph Hellwig <hch@infradead.org> Suggested-by: Christoph Hellwig <hch@infradead.org> Suggested-by: Johannes Weiner <hannes@cmpxchg.org> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- block/bio.c | 25 ++++++++++++++++--------- fs/direct-io.c | 2 ++ 2 files changed, 18 insertions(+), 9 deletions(-)