Re: [PATCHv9 6/6] iomap: Add per-block dirty state tracking to improve performance

[Ritesh Harjani (IBM) <ritesh.list@gmail.com>]

Christoph Hellwig <hch@infradead.org> writes:

> Just some nitpicks,

Sure.

> this otherwise looks fine.

Thanks for the review.

>
> First during the last patches ifs as a variable name has started
> to really annoy me and I'm not sure why.  I'd like to hear from the
> others, bu maybe just state might be a better name that flows easier?
>

Ok. Let's hear from others too.

>> +static void iomap_clear_range_dirty(struct folio *folio, size_t off, size_t len)
>> +{
>> +	struct iomap_folio_state *ifs = iomap_get_ifs(folio);
>> +
>> +	if (!ifs)
>> +		return;
>> +	iomap_ifs_clear_range_dirty(folio, ifs, off, len);
>
> Maybe just do
>
> 	if (ifs)
> 		iomap_ifs_clear_range_dirty(folio, ifs, off, len);
>
> ?

Sure.

>
> But also do we even need the ifs argument to iomap_ifs_clear_range_dirty
> after we've removed it everywhere else earlier?
>

Some of the previous discussions / reasoning behind it -

- In one of the previous discussions we discussed that functions which
has _ifs_ in their naming, then it generally should imply that we will
be working on iomap_folio_state struct. So we should pass that as a
argument.

- Also in most of these *_ifs_* functions we have "ifs" as a non-null
  function argument.

- At some places where we are calling these _ifs_ functions, we
already have derived ifs, so why not just pass it.

FYI - We dropped "ifs" argument in one of the function which is
iomap_set_range_uptodate(), because we would like this function
to work in both cases.
    1. When we have non-null folio->private (ifs)
    2. When it is null.

So API wise it looks good in my humble opinion. But sure, in
case if someone has better ideas, I can look into that.

>> +	/*
>> +	 * When we have per-block dirty tracking, there can be
>> +	 * blocks within a folio which are marked uptodate
>> +	 * but not dirty. In that case it is necessary to punch
>> +	 * out such blocks to avoid leaking any delalloc blocks.
>> +	 */
>> +	ifs = iomap_get_ifs(folio);
>> +	if (!ifs)
>> +		goto skip_ifs_punch;
>> +
>> +	last_byte = min_t(loff_t, end_byte - 1,
>> +		(folio_next_index(folio) << PAGE_SHIFT) - 1);
>> +	first_blk = offset_in_folio(folio, start_byte) >> blkbits;
>> +	last_blk = offset_in_folio(folio, last_byte) >> blkbits;
>> +	for (i = first_blk; i <= last_blk; i++) {
>> +		if (!iomap_ifs_is_block_dirty(folio, ifs, i)) {
>> +			ret = punch(inode, folio_pos(folio) + (i << blkbits),
>> +				    1 << blkbits);
>> +			if (ret)
>> +				goto out;
>> +		}
>> +	}
>> +
>> +skip_ifs_punch:
>
> And happy to hear from the others, but to me having a helper for
> all the iomap_folio_state manipulation rather than having it in
> the middle of the function and jumped over if not needed would
> improve the code structure.

I think Darrick was also pointing towards having a separate funciton.
But let's hear from him & others too. I can consider adding a separate
function for above.

Does this look good?

static int iomap_write_delalloc_ifs_punch(struct inode *inode, struct folio *folio,
		struct iomap_folio_state *ifs, loff_t start_byte, loff_t end_byte,
		int (*punch)(struct inode *inode, loff_t offset, loff_t length))

The function argument are kept similar to what we have for
iomap_write_delalloc_punch(), except maybe *punch_start_byte (which is
not required).

-ritesh