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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aNo0BCzVQxR60qeT@infradead.org>
Date: Mon, 29 Sep 2025 00:23:48 -0700
From: Christoph Hellwig <hch@...radead.org>
To: Sergey Bashirov <sergeybashirov@...il.com>
Cc: Chuck Lever <chuck.lever@...cle.com>, Jeff Layton <jlayton@...nel.org>,
	NeilBrown <neil@...wn.name>,
	Olga Kornievskaia <okorniev@...hat.com>,
	Dai Ngo <Dai.Ngo@...cle.com>, Tom Talpey <tom@...pey.com>,
	linux-nfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] NFSD: Impl multiple extents in block/scsi layoutget

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.

> Signed-off-by: Sergey Bashirov <sergeybashirov@...il.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.

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