On Mon, Jun 12, 2023 at 6:16 PM Darrick J. Wong <djwong@xxxxxxxxxx> wrote:
> 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.
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
>
