Re: [PATCH 2/2] nfs: refactor pNFS functions using clear_and_wake_up_bit

[Arnaud Bonnet]

On 7/3/26 10:55 PM, Agatha Isabelle Moreira wrote:
> On Mon, Jun 22, 2026 at 07:55:11PM +0200, Arnaud Bonnet wrote:
>> Commit 8236b0ae31c83 ("bdi: wake up concurrent wb_shutdown() callers.")
>> introduces the clear_and_wake_up_bit() helper as a wrapper for the
>> common clear -> barrier -> wake up bitops sequence.
>>
>> The file pnfs.c has several helpers with identical contents. Thus they
>> are replaced with the more recent clean_and_wake_up_bit() global helper
>> which describes accurately its effects at the call and still specifies
>> the cleared bit. This also homogenizes the code with other subsystems.
>>
>> Since the helpers are no longer used after this, they can be safely
>> removed.
> 
> 
> Hi Arnaud!
> 
>>
>> Suggested-by: Agatha Isabelle Moreira <code@xxxxxxxxxx>
>> Link: https://kernelnewbies.org/Beginner%20Cleanup%20and%20Refactor%20Tasks%20by%20Agatha%20Isabelle%20Moreira#task_007
>> Signed-off-by: Arnaud Bonnet <abo@xxxxxxxxxxx>
>> ---
>>  fs/nfs/pnfs.c | 35 ++++++++++-------------------------
>>  1 file changed, 10 insertions(+), 25 deletions(-)
>>
>> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
>> index 743467e9ba20..96ee18af1ebf 100644
>> --- a/fs/nfs/pnfs.c
>> +++ b/fs/nfs/pnfs.c
>> @@ -2100,15 +2100,6 @@ static bool pnfs_is_first_layoutget(struct pnfs_layout_hdr *lo)
>>  	return test_bit(NFS_LAYOUT_FIRST_LAYOUTGET, &lo->plh_flags);
>>  }
>>  
>> -static void pnfs_clear_first_layoutget(struct pnfs_layout_hdr *lo)
>> -{
>> -	unsigned long *bitlock = &lo->plh_flags;
>> -
>> -	clear_bit_unlock(NFS_LAYOUT_FIRST_LAYOUTGET, bitlock);
>> -	smp_mb__after_atomic();
>> -	wake_up_bit(bitlock, NFS_LAYOUT_FIRST_LAYOUTGET);
>> -}
> 
> I noticed that you found a subsystem function doing something quite
> similar to what the `clear_and_wake_up_bit()` function.
> 
> From the cleanup point of view it seems good to me, but I think some
> input from someone in the nfs subsystem would be highly appreciated to
> confirm the correctness of this refactor.
> 
> IDK if you considered or if this makes total sense, but my only concern
> here is that the maintainer *could* find the replaced function
> `pnfs_clear_first_layoutget()` preferable to be kept, since it carries
> some subsystem semantics (the `struct pnfs_layout_hdr *lo` argument and
> the NFS_LAYOUT_FIRST_LAYOUTGET bit). But I could also be completely
> wrong.
> 
> In the event that `NFS_LAYOUT_FIRST_LAYOUTGET` gets replaced by another
> constant in the future (IDK if this is a plausible possibility), by
> keeping the original function there would be only a single place to
> touch.
> 
> Let's hope this message attracts more comments from someone with the
> subsystem's expertise, which would be highly appreciated.

Hi Agatha, I agree this required consideration when doing the change,
however there is prior art with the use of clear_and_wake_up_bit and it
is pretty universally done as an open coded call. If this is not
desirable, I could resubmit (probably as a single patch) by only 
replacing the sequence in the helpers.

Some similar changes by Trond are f46f84931a0aa ("NFSv4/pNFS: Don't
call _nfs4_pnfs_v3_ds_connect multiple times") and 43d20e80e2880 ("NFS:
Fix a few more clear_bit() instances that need release semantics") and
these keep the helpers, although in those cases the functions do more
than just call clear_and_wake_up_bit.

Thanks for checking the patch!

>> -
>>  static void _add_to_server_list(struct pnfs_layout_hdr *lo,
>>  				struct nfs_server *server)
>>  {
>> @@ -2284,7 +2275,8 @@ pnfs_update_layout(struct inode *ino,
>>  					iomode, lo, lseg,
>>  					PNFS_UPDATE_LAYOUT_INVALID_OPEN);
>>  			nfs4_schedule_stateid_recovery(server, ctx->state);
>> -			pnfs_clear_first_layoutget(lo);
>> +			clear_and_wake_up_bit(NFS_LAYOUT_FIRST_LAYOUTGET,
>> +				&lo->plh_flags);
>>  			pnfs_put_layout_hdr(lo);
>>  			goto lookup_again;
>>  		}
>> @@ -2353,7 +2345,8 @@ pnfs_update_layout(struct inode *ino,
>>  			if (!exception.retry)
>>  				goto out_put_layout_hdr;
>>  			if (first)
>> -				pnfs_clear_first_layoutget(lo);
>> +				clear_and_wake_up_bit(NFS_LAYOUT_FIRST_LAYOUTGET,
>> +					&lo->plh_flags);
>>  			trace_pnfs_update_layout(ino, pos, count,
>>  				iomode, lo, lseg, PNFS_UPDATE_LAYOUT_RETRY);
>>  			pnfs_put_layout_hdr(lo);
>> @@ -2365,7 +2358,7 @@ pnfs_update_layout(struct inode *ino,
>>  
>>  out_put_layout_hdr:
>>  	if (first)
>> -		pnfs_clear_first_layoutget(lo);
>> +		clear_and_wake_up_bit(NFS_LAYOUT_FIRST_LAYOUTGET, &lo->plh_flags);
>>  	trace_pnfs_update_layout(ino, pos, count, iomode, lo, lseg,
>>  				 PNFS_UPDATE_LAYOUT_EXIT);
>>  	pnfs_put_layout_hdr(lo);
>> @@ -2457,7 +2450,7 @@ static void _lgopen_prepare_attached(struct nfs4_opendata *data,
>>  	lgp = pnfs_alloc_init_layoutget_args(ino, ctx, &current_stateid, &rng,
>>  					     nfs_io_gfp_mask());
>>  	if (!lgp) {
>> -		pnfs_clear_first_layoutget(lo);
>> +		clear_and_wake_up_bit(NFS_LAYOUT_FIRST_LAYOUTGET, &lo->plh_flags);
>>  		nfs_layoutget_end(lo);
>>  		pnfs_put_layout_hdr(lo);
>>  		return;
>> @@ -2561,7 +2554,8 @@ void nfs4_lgopen_release(struct nfs4_layoutget *lgp)
>>  {
>>  	if (lgp != NULL) {
>>  		if (lgp->lo) {
>> -			pnfs_clear_first_layoutget(lgp->lo);
>> +			clear_and_wake_up_bit(NFS_LAYOUT_FIRST_LAYOUTGET,
>> +				&lgp->lo->plh_flags);
>>  			nfs_layoutget_end(lgp->lo);
>>  		}
>>  		pnfs_layoutget_free(lgp);
>> @@ -3273,15 +3267,6 @@ pnfs_generic_pg_readpages(struct nfs_pageio_descriptor *desc)
>>  }
>>  EXPORT_SYMBOL_GPL(pnfs_generic_pg_readpages);
>>  
>> -static void pnfs_clear_layoutcommitting(struct inode *inode)
>> -{
>> -	unsigned long *bitlock = &NFS_I(inode)->flags;
>> -
>> -	clear_bit_unlock(NFS_INO_LAYOUTCOMMITTING, bitlock);
>> -	smp_mb__after_atomic();
>> -	wake_up_bit(bitlock, NFS_INO_LAYOUTCOMMITTING);
>> -}
>> -
>>  /*
>>   * There can be multiple RW segments.
>>   */
>> @@ -3306,7 +3291,7 @@ static void pnfs_list_write_lseg_done(struct inode *inode, struct list_head *lis
>>  		pnfs_put_lseg(lseg);
>>  	}
>>  
>> -	pnfs_clear_layoutcommitting(inode);
>> +	clear_and_wake_up_bit(NFS_INO_LAYOUTCOMMITTING, &NFS_I(inode)->flags);
>>  }
>>  
>>  void pnfs_set_lo_fail(struct pnfs_layout_segment *lseg)
>> @@ -3446,7 +3431,7 @@ pnfs_layoutcommit_inode(struct inode *inode, bool sync)
>>  	spin_unlock(&inode->i_lock);
>>  	kfree(data);
>>  clear_layoutcommitting:
>> -	pnfs_clear_layoutcommitting(inode);
>> +	clear_and_wake_up_bit(NFS_INO_LAYOUTCOMMITTING, &NFS_I(inode)->flags);
>>  	goto out;
>>  }
>>  EXPORT_SYMBOL_GPL(pnfs_layoutcommit_inode);
>> -- 
>> 2.53.0
>>
> 
> Other than that, both patches appear to successfuly apply and compile
> just fine.