Re: [PATCH] NFSD: Impl multiple extents in block/scsi layoutget

[Chuck Lever <chuck.lever@oracle.com>]

On 9/29/25 12:23 AM, Christoph Hellwig wrote:
> The subject line is odd.  What is impl supposed to mean?
> 
> I'd say something like:
> 
> nfsd/blocklayout: support multiple extent per LAYOUTGET
> 
> On Thu, Sep 11, 2025 at 07:02:03PM +0300, Sergey Bashirov wrote:
>> This patch allows the pNFS server to respond with multiple extents
> 
> This patch is redundant and should generally be avoided in commit log.

To put this another way, the patch description should explain "why" you
want this change -- what's the benefit going to be. And maybe a little
"how" it is done, if that isn't obvious from the diff.


>> Signed-off-by: Sergey Bashirov <sergeybashirov@gmail.com>
>> ---
>> Checked with smatch, tested on pNFS block volume setup.
> 
> Any chance we could get testing for this added to pynfs?
> 
> Also what is this patch against?  If fails to apply to linux-next for
> me.
> 
>> +/**
>> + * nfsd4_block_map_extent - get extent that covers the start of segment
>> + * @inode: inode of the file requested by the client
>> + * @fhp: handle of the file requested by the client
>> + * @seg: layout subrange requested by the client
>> + * @minlength: layout min length requested by the client
>> + * @bex: output block extent structure
>> + *
>> + * Get an extent from the file system that starts at @seg->offset or below,
>> + * but may be shorter than @seg->length.
>> + *
>> + * Return values:
>> + *   %nfs_ok: Success, @bex is initialized and valid
>> + *   %nfserr_layoutunavailable: Failed to get extent for requested @seg
>> + *   OS errors converted to NFS errors
>> + */
> 
> I'm not sure how Chuck handles this, but kerneldoc comments for static
> functions and thus not as public documentation can be really annoying
> due to the verbose formatting of the paramters.  I'd generally avoid
> them and instead of have short comments explaining the interesting bits
> about the calling convention and/or functionality.

For NFSD, the tradition is to avoid kdoc comments for static functions
unless they are the target of a virtual function call.

Static functions otherwise can have big comments. Just not kdoc style.


>> +nfsd4_block_map_extent(struct inode *inode, const struct svc_fh *fhp,
>> +		const struct nfsd4_layout_seg *seg, u64 minlength,
>> +		struct pnfs_block_extent *bex)
>>  {
>>  	struct super_block *sb = inode->i_sb;
>>  	struct iomap iomap;
>>  	u32 device_generation = 0;
>>  	int error;
>>  
>> -	if (locks_in_grace(SVC_NET(rqstp)))
>> -		return nfserr_grace;
>> -
>> -	if (seg->offset & (block_size - 1)) {
>> -		dprintk("pnfsd: I/O misaligned\n");
>> -		goto out_layoutunavailable;
>> -	}
>> -
> 
> There is a lot of code movement here mixed with functional changes,
> which makes it very hard to follow.  Any chance you could first
> split the functions, then introduce the new pnfs_block_layout structure
> and only then actually add multiple extents per layoutget?  That'll
> make it a lot easier there are no unintended changes.
> 
>> + *   %nfserr_layoutunavailable: Failed to get extents for requested @seg
>> + *   OS errors converted to NFS errors
>> + */
>> +static __be32
>> +nfsd4_block_map_segment(struct inode *inode, const struct svc_fh *fhp,
>> +		const struct nfsd4_layout_seg *seg, u64 minlength,
>> +		struct pnfs_block_layout *bl)
> 
> I struggle to see what the point of the nfsd4_block_map_segment helpers
> is, as it seems to add no real value while making the code harder to
> follow.
> 
>> +{
>> +	struct nfsd4_layout_seg subseg = *seg;
>> +	u32 i;
>> +	__be32 nfserr;
> 
> Nit: nfserr can be local in the loop.
> 
> The subextent handling confuses me a bit - there is no such thing
> as a subsegment in NFS.  I'd pass a never changing end of layout,
> a moving offset, and a constant iomode directly, which should simply
> the code quite a bit, i.e. something like:
> 
> 	u64 map_start = seg->offset;
> 	u64 seg_end = seg->offset + seg->len;
> 
> 	for (..) {
> 
> 		nfserr = nfsd4_block_map_extent(inode, fhp, ...
> 				map_start, &end);
> 		if (end > seg_end) {
> 			bl->nr_extents = i + 1;
> 			break;
> 		}
> 
> 		map_start = end;
> 	}
> 
> 
>> +nfsd4_block_proc_layoutget(struct svc_rqst *rqstp, struct inode *inode,
>> +		const struct svc_fh *fhp, struct nfsd4_layoutget *args)
>> +{
>> +	struct nfsd4_layout_seg *seg = &args->lg_seg;
>> +	u64 seg_length;
>> +	struct pnfs_block_extent *first_bex, *last_bex;
>> +	struct pnfs_block_layout *bl;
>> +	u32 nr_extents_max = PAGE_SIZE / sizeof(bl->extents[0]) - 1;
> 
> Where is that -1 coming from?  Or the magic PAGE_SIZE?
> 


-- 
Chuck Lever