Christoph Hellwig <hch@xxxxxxxxxxxxx> 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
