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

Commit Message

Ritesh Harjani (IBM) June 10, 2023, 11:39 a.m. UTC

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) <[e-mail]>
---
 fs/iomap/buffered-io.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

Comments

Christoph Hellwig June 12, 2023, 6:25 a.m. UTC | #1

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) <[e-mail]>
> ---
>  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);
> +}

A little nitpicky, but do the _ifs_ name compenents here really add
value?

Otherwise looks good:

Reviewed-by: Christoph Hellwig <[e-mail]>

Ritesh Harjani (IBM) June 12, 2023, 9:14 a.m. UTC | #2

Christoph Hellwig <[e-mail]> writes:

> 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) <[e-mail]>
>> ---
>>  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);
>> +}

static inline bool iomap_ifs_is_block_dirty(struct folio *folio,
		struct iomap_folio_state *ifs, int block)
{
	struct inode *inode = folio->mapping->host;
	unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);

	return test_bit(block + blks_per_folio, ifs->state);
}

>
> A little nitpicky, but do the _ifs_ name compenents here really add
> value?
>

Maybe if you look at both of above functions together to see the value?

> Otherwise looks good:
>
> Reviewed-by: Christoph Hellwig <[e-mail]>

Thanks!

Andreas Gruenbacher June 12, 2023, 12:40 p.m. UTC | #3

On Sat, Jun 10, 2023 at 1:39 PM Ritesh Harjani (IBM)
<[e-mail]> 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) <[e-mail]>
> ---
>  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));

This should be written as something like:

unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
return bitmap_full(ifs->state + IOMAP_ST_UPTODATE * blks_per_folio,
blks_per_folio);

> +}
> +
> +static inline bool iomap_ifs_is_block_uptodate(struct iomap_folio_state *ifs,
> +                                              unsigned int block)
> +{
> +       return test_bit(block, ifs->state);

This function should be called iomap_ifs_block_is_uptodate(), and
probably be written as follows, passing in the folio as well (this
will optimize out, anyway):

struct inode *inode = folio->mapping->host;
unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
return test_bit(block, ifs->state + IOMAP_ST_UPTODATE * blks_per_folio);

> +}
> +
>  static void iomap_ifs_set_range_uptodate(struct folio *folio,
>                 struct iomap_folio_state *ifs, size_t off, size_t len)
>  {
> @@ -54,7 +68,7 @@ static void iomap_ifs_set_range_uptodate(struct folio *folio,
>
>         spin_lock_irqsave(&ifs->state_lock, flags);
>         bitmap_set(ifs->state, first_blk, nr_blks);
> -       if (bitmap_full(ifs->state, i_blocks_per_folio(inode, folio)))
> +       if (iomap_ifs_is_fully_uptodate(folio, ifs))
>                 folio_mark_uptodate(folio);
>         spin_unlock_irqrestore(&ifs->state_lock, flags);
>  }
> @@ -99,14 +113,12 @@ static struct iomap_folio_state *iomap_ifs_alloc(struct inode *inode,
>  static void iomap_ifs_free(struct folio *folio)
>  {
>         struct iomap_folio_state *ifs = folio_detach_private(folio);
> -       struct inode *inode = folio->mapping->host;
> -       unsigned int nr_blocks = i_blocks_per_folio(inode, folio);
>
>         if (!ifs)
>                 return;
>         WARN_ON_ONCE(atomic_read(&ifs->read_bytes_pending));
>         WARN_ON_ONCE(atomic_read(&ifs->write_bytes_pending));
> -       WARN_ON_ONCE(bitmap_full(ifs->state, nr_blocks) !=
> +       WARN_ON_ONCE(iomap_ifs_is_fully_uptodate(folio, ifs) !=
>                         folio_test_uptodate(folio));
>         kfree(ifs);
>  }
> @@ -137,7 +149,7 @@ static void iomap_adjust_read_range(struct inode *inode, struct folio *folio,
>
>                 /* move forward for each leading block marked uptodate */
>                 for (i = first; i <= last; i++) {
> -                       if (!test_bit(i, ifs->state))
> +                       if (!iomap_ifs_is_block_uptodate(ifs, i))
>                                 break;
>                         *pos += block_size;
>                         poff += block_size;
> @@ -147,7 +159,7 @@ static void iomap_adjust_read_range(struct inode *inode, struct folio *folio,
>
>                 /* truncate len if we find any trailing uptodate block(s) */
>                 for ( ; i <= last; i++) {
> -                       if (test_bit(i, ifs->state)) {
> +                       if (iomap_ifs_is_block_uptodate(ifs, i)) {
>                                 plen -= (last - i + 1) * block_size;
>                                 last = i - 1;
>                                 break;
> @@ -451,7 +463,7 @@ bool iomap_is_partially_uptodate(struct folio *folio, size_t from, size_t count)
>         last = (from + count - 1) >> inode->i_blkbits;
>
>         for (i = first; i <= last; i++)
> -               if (!test_bit(i, ifs->state))
> +               if (!iomap_ifs_is_block_uptodate(ifs, i))
>                         return false;
>         return true;
>  }
> @@ -1627,7 +1639,7 @@ iomap_writepage_map(struct iomap_writepage_ctx *wpc,
>          * invalid, grab a new one.
>          */
>         for (i = 0; i < nblocks && pos < end_pos; i++, pos += len) {
> -               if (ifs && !test_bit(i, ifs->state))
> +               if (ifs && !iomap_ifs_is_block_uptodate(ifs, i))
>                         continue;
>
>                 error = wpc->ops->map_blocks(wpc, inode, pos);
> --
> 2.40.1
>

Thanks,
Andreas

Andreas Gruenbacher June 12, 2023, 12:54 p.m. UTC | #4

On Mon, Jun 12, 2023 at 8:25 AM Christoph Hellwig <[e-mail]> 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) <[e-mail]>
> > ---
> >  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().

> > +}
>
> 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

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

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

Thanks,
Andreas

Ritesh Harjani (IBM) June 12, 2023, 3:18 p.m. UTC | #5

Andreas Gruenbacher <[e-mail]> writes:

> On Mon, Jun 12, 2023 at 8:25 AM Christoph Hellwig <[e-mail]> 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) <[e-mail]>
>> > ---
>> >  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

Matthew Wilcox June 12, 2023, 3:24 p.m. UTC | #6

On Mon, Jun 12, 2023 at 08:48:16PM +0530, Ritesh Harjani wrote:
> > 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. 

Well, it doesn't because it's iomap_iomap_folio_state_is_fully_uptodate.
I don't think there's any need to namespace this so fully.
ifs_is_fully_uptodate() is just fine for a static function, IMO.

Ritesh Harjani (IBM) June 12, 2023, 3:30 p.m. UTC | #7

Andreas Gruenbacher <[e-mail]> writes:

> On Sat, Jun 10, 2023 at 1:39 PM Ritesh Harjani (IBM)
> <[e-mail]> 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) <[e-mail]>
>> ---
>>  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));
>
> This should be written as something like:
>
> unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
> return bitmap_full(ifs->state + IOMAP_ST_UPTODATE * blks_per_folio,
> blks_per_folio);
>

Nah, I feel it is not required... It make sense when we have the same
function getting use for both "uptodate" and "dirty" state.
Here the function anyways operates on uptodate state.
Hence I feel it is not required.

>> +}
>> +
>> +static inline bool iomap_ifs_is_block_uptodate(struct iomap_folio_state *ifs,
>> +                                              unsigned int block)
>> +{
>> +       return test_bit(block, ifs->state);
>
> This function should be called iomap_ifs_block_is_uptodate(), and
> probably be written as follows, passing in the folio as well (this
> will optimize out, anyway):
>
> struct inode *inode = folio->mapping->host;
> unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
> return test_bit(block, ifs->state + IOMAP_ST_UPTODATE * blks_per_folio);
>

Same here.

-ritesh

Ritesh Harjani (IBM) June 12, 2023, 3:33 p.m. UTC | #8

Matthew Wilcox <[e-mail]> writes:

> On Mon, Jun 12, 2023 at 08:48:16PM +0530, Ritesh Harjani wrote:
>> > 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.
>
> Well, it doesn't because it's iomap_iomap_folio_state_is_fully_uptodate.

:P 

> I don't think there's any need to namespace this so fully.
> ifs_is_fully_uptodate() is just fine for a static function, IMO.

Ohh, we went that road but were shot down. That time the naming was
iop_is_fully_uptodate(). :(

-ritesh

Andreas Gruenbacher June 12, 2023, 3:57 p.m. UTC | #9

On Mon, Jun 12, 2023 at 5:24 PM Matthew Wilcox <[e-mail]> wrote:
> On Mon, Jun 12, 2023 at 08:48:16PM +0530, Ritesh Harjani wrote:
> > > 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.
>
> Well, it doesn't because it's iomap_iomap_folio_state_is_fully_uptodate.

Exactly.

> I don't think there's any need to namespace this so fully.
> ifs_is_fully_uptodate() is just fine for a static function, IMO.

I'd be perfectly happy with that kind of naming scheme as well.

Thanks,
Andreas

Darrick J. Wong June 12, 2023, 4:10 p.m. UTC | #10

On Mon, Jun 12, 2023 at 05:57:48PM +0200, Andreas Gruenbacher wrote:
> On Mon, Jun 12, 2023 at 5:24 PM Matthew Wilcox <[e-mail]> wrote:
> > On Mon, Jun 12, 2023 at 08:48:16PM +0530, Ritesh Harjani wrote:
> > > > 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.
> >
> > Well, it doesn't because it's iomap_iomap_folio_state_is_fully_uptodate.
> 
> Exactly.
> 
> > I don't think there's any need to namespace this so fully.
> > ifs_is_fully_uptodate() is just fine for a static function, IMO.
> 
> I'd be perfectly happy with that kind of naming scheme as well.

Ugh, /another/ round of renaming.

to_folio_state (or just folio->private)

ifs_alloc
ifs_free
ifs_calc_range

ifs_set_range_uptodate
ifs_is_fully_uptodate
ifs_block_is_uptodate

ifs_block_is_dirty
ifs_clear_range_dirty
ifs_set_range_dirty

No more renaming suggestions.  I've reached the point where my eyes and
brain have both glazed over from repeated re-reads of this series such
that I don't see the *bugs* anymore.

Anyone else wanting new naming gets to *send in their own patch*.
Please focus only on finding code defects or friction between iomap and
some other subsystem.

Flame away about my aggressive tone,

~Your burned out and pissed off maintainer~

> Thanks,
> Andreas
>

Andreas Grünbacher June 12, 2023, 4:14 p.m. UTC | #11

Am Mo., 12. Juni 2023 um 17:43 Uhr schrieb Ritesh Harjani
<[e-mail]>:
> Andreas Gruenbacher <[e-mail]> writes:
> > On Sat, Jun 10, 2023 at 1:39 PM Ritesh Harjani (IBM)
> > <[e-mail]> 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) <[e-mail]>
> >> ---
> >>  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));
> >
> > This should be written as something like:
> >
> > unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
> > return bitmap_full(ifs->state + IOMAP_ST_UPTODATE * blks_per_folio,
> > blks_per_folio);
> >
>
> Nah, I feel it is not required... It make sense when we have the same
> function getting use for both "uptodate" and "dirty" state.
> Here the function anyways operates on uptodate state.
> Hence I feel it is not required.

So we have this iomap_block_state enum now, but IOMAP_ST_UPTODATE must
be 0 or else the code will break. That's worse than not having this
abstraction in the first place because.

Andreas

> >> +}
> >> +
> >> +static inline bool iomap_ifs_is_block_uptodate(struct iomap_folio_state *ifs,
> >> +                                              unsigned int block)
> >> +{
> >> +       return test_bit(block, ifs->state);
> >
> > This function should be called iomap_ifs_block_is_uptodate(), and
> > probably be written as follows, passing in the folio as well (this
> > will optimize out, anyway):
> >
> > struct inode *inode = folio->mapping->host;
> > unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
> > return test_bit(block, ifs->state + IOMAP_ST_UPTODATE * blks_per_folio);
> >
>
> Same here.
>
> -ritesh

Darrick J. Wong June 12, 2023, 4:16 p.m. UTC | #12

On Mon, Jun 12, 2023 at 09:00:29PM +0530, Ritesh Harjani wrote:
> Andreas Gruenbacher <[e-mail]> writes:
> 
> > On Sat, Jun 10, 2023 at 1:39 PM Ritesh Harjani (IBM)
> > <[e-mail]> 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) <[e-mail]>
> >> ---
> >>  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));
> >
> > This should be written as something like:
> >
> > unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
> > return bitmap_full(ifs->state + IOMAP_ST_UPTODATE * blks_per_folio,
> > blks_per_folio);
> >
> 
> Nah, I feel it is not required... It make sense when we have the same
> function getting use for both "uptodate" and "dirty" state.
> Here the function anyways operates on uptodate state.
> Hence I feel it is not required.

Honestly I thought that enum-for-bits thing was excessive considering
that ifs has only two state bits.  But, since you included it, it
doesn't make much sense /not/ to use it here.

OTOH, if you disassemble the object code and discover that the compiler
*isn't* using constant propagation to simplify the object code, then
yes, that would be a good reason to get rid of it.

--D

> >> +}
> >> +
> >> +static inline bool iomap_ifs_is_block_uptodate(struct iomap_folio_state *ifs,
> >> +                                              unsigned int block)
> >> +{
> >> +       return test_bit(block, ifs->state);
> >
> > This function should be called iomap_ifs_block_is_uptodate(), and
> > probably be written as follows, passing in the folio as well (this
> > will optimize out, anyway):
> >
> > struct inode *inode = folio->mapping->host;
> > unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
> > return test_bit(block, ifs->state + IOMAP_ST_UPTODATE * blks_per_folio);
> >
> 
> Same here.
> 
> -ritesh

Andreas Gruenbacher June 12, 2023, 4:19 p.m. UTC | #13

On Mon, Jun 12, 2023 at 6:16 PM Darrick J. Wong <[e-mail]> wrote:
> On Mon, Jun 12, 2023 at 09:00:29PM +0530, Ritesh Harjani wrote:
> > Andreas Gruenbacher <[e-mail]> writes:
> >
> > > On Sat, Jun 10, 2023 at 1:39 PM Ritesh Harjani (IBM)
> > > <[e-mail]> 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) <[e-mail]>
> > >> ---
> > >>  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));
> > >
> > > This should be written as something like:
> > >
> > > unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
> > > return bitmap_full(ifs->state + IOMAP_ST_UPTODATE * blks_per_folio,
> > > blks_per_folio);
> > >
> >
> > Nah, I feel it is not required... It make sense when we have the same
> > function getting use for both "uptodate" and "dirty" state.
> > Here the function anyways operates on uptodate state.
> > Hence I feel it is not required.
>
> Honestly I thought that enum-for-bits thing was excessive considering
> that ifs has only two state bits.  But, since you included it, it
> doesn't make much sense /not/ to use it here.
>
> OTOH, if you disassemble the object code and discover that the compiler
> *isn't* using constant propagation to simplify the object code, then
> yes, that would be a good reason to get rid of it.

I've checked on x86_64 earlier and there at least, the enum didn't
affect the produced code at all.

Andreas

> --D
>
> > >> +}
> > >> +
> > >> +static inline bool iomap_ifs_is_block_uptodate(struct iomap_folio_state *ifs,
> > >> +                                              unsigned int block)
> > >> +{
> > >> +       return test_bit(block, ifs->state);
> > >
> > > This function should be called iomap_ifs_block_is_uptodate(), and
> > > probably be written as follows, passing in the folio as well (this
> > > will optimize out, anyway):
> > >
> > > struct inode *inode = folio->mapping->host;
> > > unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
> > > return test_bit(block, ifs->state + IOMAP_ST_UPTODATE * blks_per_folio);
> > >
> >
> > Same here.
> >
> > -ritesh
>

Ritesh Harjani (IBM) June 12, 2023, 5:54 p.m. UTC | #14

"Darrick J. Wong" <[e-mail]> writes:

> On Mon, Jun 12, 2023 at 05:57:48PM +0200, Andreas Gruenbacher wrote:
>> On Mon, Jun 12, 2023 at 5:24 PM Matthew Wilcox <[e-mail]> wrote:
>> > On Mon, Jun 12, 2023 at 08:48:16PM +0530, Ritesh Harjani wrote:
>> > > > 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.
>> >
>> > Well, it doesn't because it's iomap_iomap_folio_state_is_fully_uptodate.
>> 
>> Exactly.
>> 
>> > I don't think there's any need to namespace this so fully.
>> > ifs_is_fully_uptodate() is just fine for a static function, IMO.
>> 
>> I'd be perfectly happy with that kind of naming scheme as well.
>
> Ugh, /another/ round of renaming.
>
> to_folio_state (or just folio->private)
>
> ifs_alloc
> ifs_free
> ifs_calc_range
>
> ifs_set_range_uptodate
> ifs_is_fully_uptodate
> ifs_block_is_uptodate
>
> ifs_block_is_dirty
> ifs_clear_range_dirty
> ifs_set_range_dirty
>

Oops you have put me into a tough spot here. 
We came back from iop_** functions naming to iomap_iop_** to
iomap_ifs_**.

Christoph? Is it ok if we go back to ifs_** functions here then? 

Or do others prefer 
iomap_folio_state_** namings. instead of ifs_**  or iomap_ifs_**? 


> No more renaming suggestions.  I've reached the point where my eyes and
> brain have both glazed over from repeated re-reads of this series such
> that I don't see the *bugs* anymore.
>
> Anyone else wanting new naming gets to *send in their own patch*.
> Please focus only on finding code defects or friction between iomap and
> some other subsystem.

Yes, it would be helpful if we uncover any bugs/ or even suggstions for
how can we better test this (adding/improving any given test in xfstests).

I have been using xfstests mainly on x86 with 1k and Power with 4k "-g auto".
I will make sure I run some more configs before sending the next
revision. 

>
> Flame away about my aggressive tone,

Thanks Darrick. No issues at all. 

>
> ~Your burned out and pissed off maintainer~
>
>> Thanks,
>> Andreas
>> 

-ritesh

Ritesh Harjani (IBM) June 12, 2023, 5:57 p.m. UTC | #15

"Darrick J. Wong" <[e-mail]> writes:

> On Mon, Jun 12, 2023 at 09:00:29PM +0530, Ritesh Harjani wrote:
>> Andreas Gruenbacher <[e-mail]> writes:
>> 
>> > On Sat, Jun 10, 2023 at 1:39 PM Ritesh Harjani (IBM)
>> > <[e-mail]> 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) <[e-mail]>
>> >> ---
>> >>  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));
>> >
>> > This should be written as something like:
>> >
>> > unsigned int blks_per_folio = i_blocks_per_folio(inode, folio);
>> > return bitmap_full(ifs->state + IOMAP_ST_UPTODATE * blks_per_folio,
>> > blks_per_folio);
>> >
>> 
>> Nah, I feel it is not required... It make sense when we have the same
>> function getting use for both "uptodate" and "dirty" state.
>> Here the function anyways operates on uptodate state.
>> Hence I feel it is not required.
>
> Honestly I thought that enum-for-bits thing was excessive considering
> that ifs has only two state bits.  But, since you included it, it
> doesn't make much sense /not/ to use it here.

Ok. Will make the changes.

>
> OTOH, if you disassemble the object code and discover that the compiler
> *isn't* using constant propagation to simplify the object code, then
> yes, that would be a good reason to get rid of it.

Sure, I will check that once too.


-ritesh

Patch

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);
+}
+
 static void iomap_ifs_set_range_uptodate(struct folio *folio,
 		struct iomap_folio_state *ifs, size_t off, size_t len)
 {
@@ -54,7 +68,7 @@  static void iomap_ifs_set_range_uptodate(struct folio *folio,
 
 	spin_lock_irqsave(&ifs->state_lock, flags);
 	bitmap_set(ifs->state, first_blk, nr_blks);
-	if (bitmap_full(ifs->state, i_blocks_per_folio(inode, folio)))
+	if (iomap_ifs_is_fully_uptodate(folio, ifs))
 		folio_mark_uptodate(folio);
 	spin_unlock_irqrestore(&ifs->state_lock, flags);
 }
@@ -99,14 +113,12 @@  static struct iomap_folio_state *iomap_ifs_alloc(struct inode *inode,
 static void iomap_ifs_free(struct folio *folio)
 {
 	struct iomap_folio_state *ifs = folio_detach_private(folio);
-	struct inode *inode = folio->mapping->host;
-	unsigned int nr_blocks = i_blocks_per_folio(inode, folio);
 
 	if (!ifs)
 		return;
 	WARN_ON_ONCE(atomic_read(&ifs->read_bytes_pending));
 	WARN_ON_ONCE(atomic_read(&ifs->write_bytes_pending));
-	WARN_ON_ONCE(bitmap_full(ifs->state, nr_blocks) !=
+	WARN_ON_ONCE(iomap_ifs_is_fully_uptodate(folio, ifs) !=
 			folio_test_uptodate(folio));
 	kfree(ifs);
 }
@@ -137,7 +149,7 @@  static void iomap_adjust_read_range(struct inode *inode, struct folio *folio,
 
 		/* move forward for each leading block marked uptodate */
 		for (i = first; i <= last; i++) {
-			if (!test_bit(i, ifs->state))
+			if (!iomap_ifs_is_block_uptodate(ifs, i))
 				break;
 			*pos += block_size;
 			poff += block_size;
@@ -147,7 +159,7 @@  static void iomap_adjust_read_range(struct inode *inode, struct folio *folio,
 
 		/* truncate len if we find any trailing uptodate block(s) */
 		for ( ; i <= last; i++) {
-			if (test_bit(i, ifs->state)) {
+			if (iomap_ifs_is_block_uptodate(ifs, i)) {
 				plen -= (last - i + 1) * block_size;
 				last = i - 1;
 				break;
@@ -451,7 +463,7 @@  bool iomap_is_partially_uptodate(struct folio *folio, size_t from, size_t count)
 	last = (from + count - 1) >> inode->i_blkbits;
 
 	for (i = first; i <= last; i++)
-		if (!test_bit(i, ifs->state))
+		if (!iomap_ifs_is_block_uptodate(ifs, i))
 			return false;
 	return true;
 }
@@ -1627,7 +1639,7 @@  iomap_writepage_map(struct iomap_writepage_ctx *wpc,
 	 * invalid, grab a new one.
 	 */
 	for (i = 0; i < nblocks && pos < end_pos; i++, pos += len) {
-		if (ifs && !test_bit(i, ifs->state))
+		if (ifs && !iomap_ifs_is_block_uptodate(ifs, i))
 			continue;
 
 		error = wpc->ops->map_blocks(wpc, inode, pos);