Matthew Wilcox <willy@xxxxxxxxxxxxx> writes:
> On Fri, Apr 26, 2024 at 11:25:25PM +0530, Ritesh Harjani wrote:
>> Matthew Wilcox <willy@xxxxxxxxxxxxx> writes:
>> > The approach I suggested was to initialise read_bytes_pending to
>> > folio_size() at the start. Then subtract off blocksize for each
>> > uptodate block, whether you find it already uptodate, or as the
>> > completion handler runs.
>> >
>> > Is there a reason that doesn't work?
>>
>> That is what this patch series does right. The current patch does work
>> as far as my testing goes.
>>
>> For e.g. This is what initializes the r_b_p for the first time when
>> ifs->r_b_p is 0.
>>
>> + loff_t to_read = min_t(loff_t, iter->len - offset,
>> + folio_size(folio) - offset_in_folio(folio, orig_pos));
>> <..>
>> + if (!ifs->read_bytes_pending)
>> + ifs->read_bytes_pending = to_read;
>>
>>
>> Then this is where we subtract r_b_p for blocks which are uptodate.
>> + padjust = pos - orig_pos;
>> + ifs->read_bytes_pending -= padjust;
>>
>>
>> This is when we adjust r_b_p when we directly zero the folio.
>> if (iomap_block_needs_zeroing(iter, pos)) {
>> + if (ifs) {
>> + spin_lock_irq(&ifs->state_lock);
>> + ifs->read_bytes_pending -= plen;
>> + if (!ifs->read_bytes_pending)
>> + rbp_finished = true;
>> + spin_unlock_irq(&ifs->state_lock);
>> + }
>>
>> But as you see this requires surgery throughout read paths. What if
>> we add a state flag to ifs only for BH_BOUNDARY. Maybe that could
>> result in a more simplified approach?
>> Because all we require is to know whether the folio should be unlocked
>> or not at the time of completion.
>>
>> Do you think we should try that part or you think the current approach
>> looks ok?
>
> You've really made life hard for yourself. I had something more like
> this in mind. I may have missed a few places that need to be changed,
> but this should update read_bytes_pending everwhere that we set bits
> in the uptodate bitmap, so it should be right?
Please correct me if I am wrong.
>
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index 41c8f0c68ef5..f87ca8ee4d19 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -79,6 +79,7 @@ static void iomap_set_range_uptodate(struct folio *folio, size_t off,
> if (ifs) {
> spin_lock_irqsave(&ifs->state_lock, flags);
> uptodate = ifs_set_range_uptodate(folio, ifs, off, len);
> + ifs->read_bytes_pending -= len;
> spin_unlock_irqrestore(&ifs->state_lock, flags);
> }
iomap_set_range_uptodate() gets called from ->write_begin() and
->write_end() too. So what we are saying is we are updating
the state of read_bytes_pending even though we are not in
->read_folio() or ->readahead() call?
>
> @@ -208,6 +209,8 @@ static struct iomap_folio_state *ifs_alloc(struct inode *inode,
> spin_lock_init(&ifs->state_lock);
> if (folio_test_uptodate(folio))
> bitmap_set(ifs->state, 0, nr_blocks);
> + else
> + ifs->read_bytes_pending = folio_size(folio);
We might not come till here during ->read_folio -> ifs_alloc(). Since we
might have a cached ifs which was allocated during write to this folio.
But unless you are saying that during writes, we would have set
ifs->r_b_p to folio_size() and when the read call happens, we use
the same value of the cached ifs.
Ok, I see. I was mostly focusing on updating ifs->r_b_p value only when
the reads bytes are actually pending during ->read_folio() or
->readahead() and not updating r_b_p during writes.
...One small problem which I see with this approach is - we might have
some non-zero value in ifs->r_b_p when ifs_free() gets called and it
might give a warning of non-zero ifs->r_b_p, because we updated
ifs->r_b_p during writes to a non-zero value, but the reads
never happend. Then during a call to ->release_folio, it will complain
of a non-zero ifs->r_b_p.
> if (folio_test_dirty(folio))
> bitmap_set(ifs->state, nr_blocks, nr_blocks);
> folio_attach_private(folio, ifs);
> @@ -396,12 +399,6 @@ static loff_t iomap_readpage_iter(const struct iomap_iter *iter,
> }
>
> ctx->cur_folio_in_bio = true;
> - if (ifs) {
> - spin_lock_irq(&ifs->state_lock);
> - ifs->read_bytes_pending += plen;
> - spin_unlock_irq(&ifs->state_lock);
> - }
> -
> sector = iomap_sector(iomap, pos);
> if (!ctx->bio ||
> bio_end_sector(ctx->bio) != sector ||
-ritesh
