Re: [PATCH v1 4/6] block/psi: remove PSI annotations from direct IO

2020-12-22 Thread Christoph Hellwig 
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.

Re: [PATCH v1 4/6] block/psi: remove PSI annotations from direct IO

2020-12-15 Thread Pavel Begunkov 



On 15/12/2020 01:33, Dave Chinner wrote:
> On Tue, Dec 15, 2020 at 01:03:45AM +, Pavel Begunkov wrote:
>> On 15/12/2020 00:56, Dave Chinner wrote:
>>> On Tue, Dec 15, 2020 at 12:20:23AM +, 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 
 Suggested-by: Christoph Hellwig 
 Suggested-by: Johannes Weiner 
 Signed-off-by: Pavel Begunkov 
 ---
  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  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.
> 

-- 
Pavel Begunkov

Re: [PATCH v1 4/6] block/psi: remove PSI annotations from direct IO

2020-12-14 Thread Dave Chinner 
On Tue, Dec 15, 2020 at 01:03:45AM +, Pavel Begunkov wrote:
> On 15/12/2020 00:56, Dave Chinner wrote:
> > On Tue, Dec 15, 2020 at 12:20:23AM +, 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 
> >> Suggested-by: Christoph Hellwig 
> >> Suggested-by: Johannes Weiner 
> >> Signed-off-by: Pavel Begunkov 
> >> ---
> >>  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  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.
-- 
Dave Chinner
da...@fromorbit.com

Re: [PATCH v1 4/6] block/psi: remove PSI annotations from direct IO

2020-12-14 Thread Pavel Begunkov 
On 15/12/2020 00:56, Dave Chinner wrote:
> On Tue, Dec 15, 2020 at 12:20:23AM +, 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 
>> Suggested-by: Christoph Hellwig 
>> Suggested-by: Johannes Weiner 
>> Signed-off-by: Pavel Begunkov 
>> ---
>>  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. 

-- 
Pavel Begunkov

Re: [PATCH v1 4/6] block/psi: remove PSI annotations from direct IO

2020-12-14 Thread Dave Chinner 
On Tue, Dec 15, 2020 at 12:20:23AM +, 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 
> Suggested-by: Christoph Hellwig 
> Suggested-by: Johannes Weiner 
> Signed-off-by: Pavel Begunkov 
> ---
>  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.
-- 
Dave Chinner
da...@fromorbit.com

[PATCH v1 4/6] block/psi: remove PSI annotations from direct IO

2020-12-14 Thread Pavel Begunkov 
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 
Suggested-by: Christoph Hellwig 
Suggested-by: Johannes Weiner 
Signed-off-by: Pavel Begunkov 
---
 block/bio.c| 25 -
 fs/direct-io.c |  2 ++
 2 files changed, 18 insertions(+), 9 deletions(-)

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, _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(>bio_lock, flags);
dio->refcount++;
-- 
2.24.0