Re: [RFCv2 2/3] iomap: Change uptodate variable name to state

[Dave Chinner <david@fromorbit.com>]

On Mon, Jan 30, 2023 at 09:44:12PM +0530, Ritesh Harjani (IBM) wrote:
> This patch just changes the struct iomap_page uptodate & uptodate_lock
> member names to state and state_lock to better reflect their purpose for
> the upcoming patch.
> 
> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> ---
>  fs/iomap/buffered-io.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index e9c85fcf7a1f..faee2852db8f 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -25,13 +25,13 @@
>  
>  /*
>   * Structure allocated for each folio when block size < folio size
> - * to track sub-folio uptodate status and I/O completions.
> + * to track sub-folio uptodate state and I/O completions.
>   */
>  struct iomap_page {
>  	atomic_t		read_bytes_pending;
>  	atomic_t		write_bytes_pending;
> -	spinlock_t		uptodate_lock;
> -	unsigned long		uptodate[];
> +	spinlock_t		state_lock;
> +	unsigned long		state[];

I don't realy like this change, nor the followup in the next patch
that puts two different state bits somewhere inside a single bitmap.

>  };
>  
>  static inline struct iomap_page *to_iomap_page(struct folio *folio)
> @@ -58,12 +58,12 @@ iomap_page_create(struct inode *inode, struct folio *folio, unsigned int flags)
>  	else
>  		gfp = GFP_NOFS | __GFP_NOFAIL;
>  
> -	iop = kzalloc(struct_size(iop, uptodate, BITS_TO_LONGS(nr_blocks)),
> +	iop = kzalloc(struct_size(iop, state, BITS_TO_LONGS(nr_blocks)),
>  		      gfp);
>  	if (iop) {
> -		spin_lock_init(&iop->uptodate_lock);
> +		spin_lock_init(&iop->state_lock);
>  		if (folio_test_uptodate(folio))
> -			bitmap_fill(iop->uptodate, nr_blocks);
> +			bitmap_fill(iop->state, nr_blocks);

This is the reason I don't like it - we lose the self-documenting
aspect of the code. bitmap_fill(iop->uptodate, nr_blocks) is
obviously correct, the new version isn't because "state" has no
obvious meaning, and it only gets worse in the next patch where
state is changed to have a magic "2 * nr_blocks" length and multiple
state bits per block.

Having an obvious setup where there are two bitmaps, one for dirty
and one for uptodate, and the same bit in each bitmap corresponds to
the state for that sub-block region, it is easy to see that the code
is operating on the correct bit, to look at the bitmap and see what
bits are set, to compare uptodate and dirty bitmaps side by side,
etc. It's a much easier setup to read, code correctly, analyse and
debug than putting multiple state bits in the same bitmap array at
different indexes.

If you are trying to keep this down to a single allocation by using
a single bitmap of undefined length, then change the declaration and
the structure size calculation away from using array notation and
instead just use pointers to the individual bitmap regions within
the allocated region.

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com