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