Re: [PATCHv9 3/6] iomap: Add some uptodate state handling helpers for ifs state bitmap

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

Andreas Gruenbacher <agruenba@redhat.com> writes:

> On Mon, Jun 12, 2023 at 8:25 AM Christoph Hellwig <hch@infradead.org> wrote:
>> On Sat, Jun 10, 2023 at 05:09:04PM +0530, Ritesh Harjani (IBM) wrote:
>> > This patch adds two of the helper routines iomap_ifs_is_fully_uptodate()
>> > and iomap_ifs_is_block_uptodate() for managing uptodate state of
>> > ifs state bitmap.
>> >
>> > In later patches ifs state bitmap array will also handle dirty state of all
>> > blocks of a folio. Hence this patch adds some helper routines for handling
>> > uptodate state of the ifs state bitmap.
>> >
>> > Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>> > ---
>> >  fs/iomap/buffered-io.c | 28 ++++++++++++++++++++--------
>> >  1 file changed, 20 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
>> > index e237f2b786bc..206808f6e818 100644
>> > --- a/fs/iomap/buffered-io.c
>> > +++ b/fs/iomap/buffered-io.c
>> > @@ -43,6 +43,20 @@ static inline struct iomap_folio_state *iomap_get_ifs(struct folio *folio)
>> >
>> >  static struct bio_set iomap_ioend_bioset;
>> >
>> > +static inline bool iomap_ifs_is_fully_uptodate(struct folio *folio,
>> > +                                            struct iomap_folio_state *ifs)
>> > +{
>> > +     struct inode *inode = folio->mapping->host;
>> > +
>> > +     return bitmap_full(ifs->state, i_blocks_per_folio(inode, folio));
>> > +}
>> > +
>> > +static inline bool iomap_ifs_is_block_uptodate(struct iomap_folio_state *ifs,
>> > +                                            unsigned int block)
>> > +{
>> > +     return test_bit(block, ifs->state);
>
> "block_is_uptodate" instead of "is_block_uptodate" here as well, please.
>
> Also see by previous mail about iomap_ifs_is_block_uptodate().
>

"is_block_uptodate" is what we decided in our previous discussion here [1]
[1]: https://lore.kernel.org/linux-xfs/20230605225434.GF1325469@frogsfrogsfrogs/


Unless any strong objections, for now I will keep Maintainer's suggested name
;) and wait for his feedback on this.

>> > +}
>>
>> A little nitpicky, but do the _ifs_ name compenents here really add
>> value?
>
> Since we're at the nitpicking, I don't find those names very useful,
> either. How about the following instead?
>
> iomap_ifs_alloc -> iomap_folio_state_alloc
> iomap_ifs_free -> iomap_folio_state_free
> iomap_ifs_calc_range -> iomap_folio_state_calc_range

First of all I think we need to get used to the name "ifs" like how we
were using "iop" earlier. ifs == iomap_folio_state...

>
> iomap_ifs_is_fully_uptodate -> iomap_folio_is_fully_uptodate
> iomap_ifs_is_block_uptodate -> iomap_block_is_uptodate
> iomap_ifs_is_block_dirty -> iomap_block_is_dirty
>

...if you then look above functions with _ifs_ == _iomap_folio_state_
naming. It will make more sense. 


> iomap_ifs_set_range_uptodate -> __iomap_set_range_uptodate
> iomap_ifs_clear_range_dirty -> __iomap_clear_range_dirty
> iomap_ifs_set_range_dirty -> __iomap_set_range_dirty

Same as above.

>
> Thanks,
> Andreas

Thanks for the review! 

I am not saying I am not open to changing the name. But AFAIR, Darrick
and Matthew were ok with "ifs" naming. In fact they first suggested it
as well. So if others also dislike "ifs" naming, then we could consider
other options.

-ritesh