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] [day] [month] [year] [list]

Message-ID: <qddd2ydxgzhx72ybsxt2o3o2x73wkntqsfpg57fsbw5qlfgm4l@yzan3q4ybywx>
Date: Mon, 29 Sep 2025 22:13:24 +0300
From: Sergey Bashirov <sergeybashirov@...il.com>
To: Christoph Hellwig <hch@...radead.org>, 
	Chuck Lever <chuck.lever@...cle.com>
Cc: 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

On Mon, Sep 29, 2025 at 12:23:48AM -0700, 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

Impl is short for implement. But let's rephrase that if it looks odd.

> On Thu, Sep 11, 2025 at 07:02:03PM +0300, Sergey Bashirov wrote:
> > Checked with smatch, tested on pNFS block volume setup.
> 
> Any chance we could get testing for this added to pynfs?

Thanks for pointing out pynfs! I'm not familiar with it yet, but adding
tests is a good idea. Will take a look on it.

> Also what is this patch against?  If fails to apply to linux-next for
> me.

At the time of submission, the patch was made on top of nfsd-testing.

> 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.

Agree. I will try to refactor or split the patch to reduce unnecessary
code movement produced by diff tool.

> 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

The main idea of nfsd4_block_map_segment is to logically separate work with
XFS, handling single extent, from the loop through the pnfs_block_layout
structure across all extents. If we combine nfsd4_block_map_segment and
nfsd4_block_map_extent, the switch statement will end up inside the for
loop, which will result in a large code movement in diff due to the right
shifts and line breaks.

> 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:

You are right, subsegment here simply means a smaller portion of the
original requested segment and is not related to NFS structures or
specification. Actually, I first made a version with three separate
arguments. But then reworked it to reduce the number of arguments in the
nfsd4_block_map_extent function. I am ok with any option, let's pass
three variables without using structure.

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

PAGE_SIZE is the maximum possible size of the pnfs_block_layout
structure in the server's memory that we would like to have. And -1
ensures that the nr_extents field will also fit in it
if PAGE_SIZE % sizeof(struct pnfs_block_extent) == 0.

Yes, it would be nice to add a comment in the code, or even better,
an inline function that calculates the number of extents in a given
memory limit for the pnfs_block_layout structure.

I will prepare new patch(es).

--
Sergey Bashirov