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

From: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
To: Andreas Gruenbacher <agruenba@redhat.com>
Cc: linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	Christoph Hellwig <hch@infradead.org>,
	"Darrick J. Wong" <djwong@kernel.org>,
	Matthew Wilcox <willy@infradead.org>,
	Dave Chinner <david@fromorbit.com>,
	Brian Foster <bfoster@redhat.com>,
	Ojaswin Mujoo <ojaswin@linux.ibm.com>,
	Disha Goel <disgoel@linux.ibm.com>
Subject: Re: [PATCHv9 3/6] iomap: Add some uptodate state handling helpers for ifs state bitmap
Date: Mon, 12 Jun 2023 21:00:29 +0530	[thread overview]
Message-ID: <87ilbshf56.fsf@doe.com> (raw)
In-Reply-To: <CAHc6FU7Hv71ujeb9oEVOD+bpddMMT0KY+KKUp881Am15u-OVvg@mail.gmail.com>

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.

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