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

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Jun 12, 2023 at 09:00:29PM +0530, Ritesh Harjani wrote:
> Andreas Gruenbacher <agruenba@redhat.com> writes:
> 
> > On Sat, Jun 10, 2023 at 1:39 PM Ritesh Harjani (IBM)
> > <ritesh.list@gmail.com> 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));
> >
> > 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