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

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

"Darrick J. Wong" <djwong@kernel.org> writes:

> 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.

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