* Re: [PATCH RFC v2] fhandle: expose u64 mount id to name_to_handle_at(2)
From: Christian Brauner @ 2024-05-28 7:12 UTC (permalink / raw)
To: hch@infradead.org
Cc: Trond Myklebust, jack@suse.cz, chuck.lever@oracle.com,
linux-api@vger.kernel.org, linux-fsdevel@vger.kernel.org,
linux-kernel@vger.kernel.org, alex.aring@gmail.com,
cyphar@cyphar.com, viro@zeniv.linux.org.uk, jlayton@kernel.org,
amir73il@gmail.com, linux-nfs@vger.kernel.org
In-Reply-To: <ZlS0_DWzGk24GYZA@infradead.org>
On Mon, May 27, 2024 at 09:29:48AM -0700, hch@infradead.org wrote:
> On Mon, May 27, 2024 at 03:38:40PM +0000, Trond Myklebust wrote:
> > > It
> > > does not matter what mount you use to access it.
> >
> > Sure. However if you are providing a path argument, then presumably you
> > need to know which file system (aka super_block) it eventually resolves
> > to.
>
> Except that you can't, at least not without running into potential
> races. The only way to fix a race vs unmount/remount is to include
> the fsid part in the kernel generated file handle.
>
> >
> > If your use case isn't NFS servers, then what use case are you
> > targeting, and how do you expect those applications to use this API?
>
> The main user of the open by handle syscalls seems to be fanotify
> magic.
It's also used by userspace for uniquely identifying cgroups via handles
as cgroups and - even without open_by_handle_at() - to check whether a
file is still valid.
And again a 64bit mount is is a simple way to race-free go to whatever
superblock uuid you want. They cannot be recycled and are unique for the
lifetime of the system.
^ permalink raw reply
* Re: [PATCH RFC v2] fhandle: expose u64 mount id to name_to_handle_at(2)
From: Christian Brauner @ 2024-05-28 7:05 UTC (permalink / raw)
To: Trond Myklebust
Cc: hch@infradead.org, jack@suse.cz, chuck.lever@oracle.com,
linux-fsdevel@vger.kernel.org, linux-api@vger.kernel.org,
alex.aring@gmail.com, linux-kernel@vger.kernel.org,
cyphar@cyphar.com, viro@zeniv.linux.org.uk, jlayton@kernel.org,
amir73il@gmail.com, linux-nfs@vger.kernel.org
In-Reply-To: <49b6c50a50e517b6eb61d40af6fd1fd6e9c09cb2.camel@hammerspace.com>
On Mon, May 27, 2024 at 03:47:33PM +0000, Trond Myklebust wrote:
> On Mon, 2024-05-27 at 15:17 +0200, Christian Brauner wrote:
> >
> > Returning the 64bit mount id makes this race-free because we now have
> > statmount():
> >
> > u64 mnt_id = 0;
> > name_to_handle_at(AT_FDCWD, "/path/to/file", &handle, &mnt_id, 0);
> > statmount(mnt_id);
> >
> > Which gets you the device number which one can use to figure out the
> > uuid without ever having to open a single file (We could even expose
> > the
> > UUID of the filesystem through statmount() if we wanted to.).
> >
>
> It is not race free. statmount() depends on the filesystem still being
> mounted somewhere in your namespace, which is not guaranteed above.
The unsigned 64bit mount is not recyclable. It is a unique identifier
for a mount for the lifetime of the system. Even if bumped on every
cycle it will still take hundreds of years to overflow.
^ permalink raw reply
* Re: [PATCH RFC v2] fhandle: expose u64 mount id to name_to_handle_at(2)
From: hch @ 2024-05-27 16:29 UTC (permalink / raw)
To: Trond Myklebust
Cc: hch@infradead.org, jack@suse.cz, chuck.lever@oracle.com,
linux-api@vger.kernel.org, linux-fsdevel@vger.kernel.org,
brauner@kernel.org, linux-kernel@vger.kernel.org,
alex.aring@gmail.com, cyphar@cyphar.com, viro@zeniv.linux.org.uk,
jlayton@kernel.org, amir73il@gmail.com, linux-nfs@vger.kernel.org
In-Reply-To: <86065f6a4f3d2f3d78f39e7a276a2d6e25bfbc9d.camel@hammerspace.com>
On Mon, May 27, 2024 at 03:38:40PM +0000, Trond Myklebust wrote:
> > It
> > does not matter what mount you use to access it.
>
> Sure. However if you are providing a path argument, then presumably you
> need to know which file system (aka super_block) it eventually resolves
> to.
Except that you can't, at least not without running into potential
races. The only way to fix a race vs unmount/remount is to include
the fsid part in the kernel generated file handle.
>
> If your use case isn't NFS servers, then what use case are you
> targeting, and how do you expect those applications to use this API?
The main user of the open by handle syscalls seems to be fanotify
magic.
^ permalink raw reply
* Re: [PATCH RFC v2] fhandle: expose u64 mount id to name_to_handle_at(2)
From: Christoph Hellwig @ 2024-05-27 16:24 UTC (permalink / raw)
To: Jan Kara
Cc: Christoph Hellwig, Aleksa Sarai, Alexander Viro,
Christian Brauner, Chuck Lever, Jeff Layton, Amir Goldstein,
Alexander Aring, linux-fsdevel, linux-nfs, linux-kernel,
linux-api
In-Reply-To: <20240527133430.ifjo2kksoehtuwrn@quack3>
On Mon, May 27, 2024 at 03:34:30PM +0200, Jan Kara wrote:
> So I was wondering how this is actually working in practice. Checking the
> code, NFS server is (based on configuration in /etc/exports) either using
> device number as the filesystem identifier or fsid / uuid as specified in
> /etc/exports.
Yes, it's a rather suboptimal implementation.
> So returning the 64-bit mount ID from name_to_handle_at() weasels out of
> these "how to identify arbitrary superblock" problems by giving userspace a
> reasonably reliable way to generate this superblock identifier itself. I'm
> fully open to less errorprone API for this but at this point I don't see it
> so changing the mount ID returned from name_to_handle_at() to 64-bit unique
> one seems like a sane practical choice to me...
Well, how about we fix the thing for real:
- allow file systems to provide a uniqueu identifier of at least
uuid size (16 bytes) in the superblock or through an export operation
- for non-persistent file systems allow to generate one at boot time
using the normal uuid generation helpers
- add a new flag to name_to_handle_at/open_by_handle_at to include it
in the file handle, and thus make the file handle work more like
the normal file handle
- add code to nfsd to directly make use of this
This would solve all the problems in this proposal as well as all the
obvious ones it doesn't solve.
^ permalink raw reply
* Re: [PATCH RFC v2] fhandle: expose u64 mount id to name_to_handle_at(2)
From: Christoph Hellwig @ 2024-05-27 16:18 UTC (permalink / raw)
To: Christian Brauner
Cc: Christoph Hellwig, Aleksa Sarai, Alexander Viro, Jan Kara,
Chuck Lever, Jeff Layton, Amir Goldstein, Alexander Aring,
linux-fsdevel, linux-nfs, linux-kernel, linux-api
In-Reply-To: <20240527-hagel-thunfisch-75781b0cf75d@brauner>
On Mon, May 27, 2024 at 02:29:02PM +0200, Christian Brauner wrote:
> I see not inherent problem with exposing the 64 bit mount id through
> name_to_handle_at() as we already to expose the old one anyway.
That's one way to frame it. The other is that exposing the different
mount ID also doesn't substantially fix the problem, so it's not
worth the churn.
^ permalink raw reply
* Re: [PATCH RFC v2] fhandle: expose u64 mount id to name_to_handle_at(2)
From: Trond Myklebust @ 2024-05-27 15:47 UTC (permalink / raw)
To: brauner@kernel.org, hch@infradead.org
Cc: jack@suse.cz, chuck.lever@oracle.com,
linux-fsdevel@vger.kernel.org, linux-api@vger.kernel.org,
alex.aring@gmail.com, linux-kernel@vger.kernel.org,
cyphar@cyphar.com, viro@zeniv.linux.org.uk, jlayton@kernel.org,
amir73il@gmail.com, linux-nfs@vger.kernel.org
In-Reply-To: <20240527-raufen-skorpion-fa81805b3273@brauner>
On Mon, 2024-05-27 at 15:17 +0200, Christian Brauner wrote:
>
> Returning the 64bit mount id makes this race-free because we now have
> statmount():
>
> u64 mnt_id = 0;
> name_to_handle_at(AT_FDCWD, "/path/to/file", &handle, &mnt_id, 0);
> statmount(mnt_id);
>
> Which gets you the device number which one can use to figure out the
> uuid without ever having to open a single file (We could even expose
> the
> UUID of the filesystem through statmount() if we wanted to.).
>
It is not race free. statmount() depends on the filesystem still being
mounted somewhere in your namespace, which is not guaranteed above.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com
^ permalink raw reply
* Re: [PATCH RFC v2] fhandle: expose u64 mount id to name_to_handle_at(2)
From: Trond Myklebust @ 2024-05-27 15:38 UTC (permalink / raw)
To: hch@infradead.org
Cc: jack@suse.cz, chuck.lever@oracle.com, linux-api@vger.kernel.org,
linux-fsdevel@vger.kernel.org, brauner@kernel.org,
linux-kernel@vger.kernel.org, alex.aring@gmail.com,
cyphar@cyphar.com, viro@zeniv.linux.org.uk, jlayton@kernel.org,
amir73il@gmail.com, linux-nfs@vger.kernel.org
In-Reply-To: <ZlRzNquWNalhYtux@infradead.org>
On Mon, 2024-05-27 at 04:49 -0700, hch@infradead.org wrote:
> On Sun, May 26, 2024 at 10:32:39PM +0000, Trond Myklebust wrote:
> > I assume the reason is to give the caller a race free way to figure
> > out
> > which submount the path resolves to.
>
> But the handle op are global to the file systems (aka super_block).
> It
> does not matter what mount you use to access it.
Sure. However if you are providing a path argument, then presumably you
need to know which file system (aka super_block) it eventually resolves
to.
>
> Snip random things about userland NFS servers I couldn't care less
> about..
>
My point was that at least for that case, you are better off using a
file descriptor and not having to care about the mount id.
If your use case isn't NFS servers, then what use case are you
targeting, and how do you expect those applications to use this API?
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com
^ permalink raw reply
* Re: [PATCH RFC v2] fhandle: expose u64 mount id to name_to_handle_at(2)
From: Jan Kara @ 2024-05-27 13:34 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Aleksa Sarai, Alexander Viro, Christian Brauner, Jan Kara,
Chuck Lever, Jeff Layton, Amir Goldstein, Alexander Aring,
linux-fsdevel, linux-nfs, linux-kernel, linux-api
In-Reply-To: <ZlRy7EBaV04F2UaI@infradead.org>
On Mon 27-05-24 04:47:56, Christoph Hellwig wrote:
> On Sun, May 26, 2024 at 12:01:08PM -0700, Aleksa Sarai wrote:
> > The existing interface already provides a mount ID which is not even
> > safe without rebooting.
>
> And that seems to be a big part of the problem where the Linux by handle
> syscall API deviated from all know precedence for no good reason. NFS
> file handles which were the start of this do (and have to) encode a
> persistent file system identifier. As do the xfs handles (although they
> do the decoding in the userspace library on Linux for historic reasons),
> as do the FreeBSD equivalents to these syscalls.
So I was wondering how this is actually working in practice. Checking the
code, NFS server is (based on configuration in /etc/exports) either using
device number as the filesystem identifier or fsid / uuid as specified in
/etc/exports.
> > An alternative would be to return something unique to the filesystem
> > superblock, but as far as I can tell there is no guarantee that every
> > Linux filesystem's fsid is sufficiently unique to act as a globally
> > unique identifier. At least with a 64-bit mount ID and statmount(2),
> > userspace can decide what information is needed to get sufficiently
> > unique information about the source filesystem.
>
> Well, every file system that supports export ops already needs a
> globally unique ID for NFS to work properly. We might not have good
> enough interfaces for that, but that shouldn't be too hard.
So as my research above shows, this ID is either manually configured in
/etc/exports or NFS server uses device number which is not guaranteed to be
persistent. Filesystems, at least currently, have no obligation to provide
anything (and some of them indeed don't provide any uuid or persistent
fsid). I guess that's the reason why mount ID is reported with
name_to_handle_at().
Don't get me wrong, I agree with your reservations towards mount ID (per
mount instead of per-sb, non-persistent) and I agree the properties you
describe are the golden standard we should strive for but I mainly wanted
to point out this is not reality today and in particular providing the
"persistency" guarantee of the filesystem ID may require on disk format
changes for some filesystems.
So returning the 64-bit mount ID from name_to_handle_at() weasels out of
these "how to identify arbitrary superblock" problems by giving userspace a
reasonably reliable way to generate this superblock identifier itself. I'm
fully open to less errorprone API for this but at this point I don't see it
so changing the mount ID returned from name_to_handle_at() to 64-bit unique
one seems like a sane practical choice to me...
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH RFC v2] fhandle: expose u64 mount id to name_to_handle_at(2)
From: Christian Brauner @ 2024-05-27 13:17 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Aleksa Sarai, Alexander Viro, Jan Kara, Chuck Lever, Jeff Layton,
Amir Goldstein, Alexander Aring, linux-fsdevel, linux-nfs,
linux-kernel, linux-api
In-Reply-To: <20240527-hagel-thunfisch-75781b0cf75d@brauner>
On Mon, May 27, 2024 at 02:29:02PM +0200, Christian Brauner wrote:
> On Mon, May 27, 2024 at 04:47:56AM -0700, Christoph Hellwig wrote:
> > On Sun, May 26, 2024 at 12:01:08PM -0700, Aleksa Sarai wrote:
> > > The existing interface already provides a mount ID which is not even
> > > safe without rebooting.
> >
> > And that seems to be a big part of the problem where the Linux by handle
> > syscall API deviated from all know precedence for no good reason. NFS
> > file handles which were the start of this do (and have to) encode a
> > persistent file system identifier. As do the xfs handles (although they
> > do the decoding in the userspace library on Linux for historic reasons),
> > as do the FreeBSD equivalents to these syscalls.
> >
> > > An alternative would be to return something unique to the filesystem
> > > superblock, but as far as I can tell there is no guarantee that every
> > > Linux filesystem's fsid is sufficiently unique to act as a globally
> > > unique identifier. At least with a 64-bit mount ID and statmount(2),
> > > userspace can decide what information is needed to get sufficiently
> > > unique information about the source filesystem.
> >
> > Well, every file system that supports export ops already needs a
> > globally unique ID for NFS to work properly. We might not have good
> > enough interfaces for that, but that shouldn't be too hard.
>
> I see not inherent problem with exposing the 64 bit mount id through
> name_to_handle_at() as we already to expose the old one anyway.
>
> But I agree that it is useful if we had the guarantee that file handles
> are unique in the way you describe. As it currently stands that doesn't
> seem to be the case and userspace doesn't seem to have a way of figuring
> out if the handle provided by name_to_handle_at() is indeed unique as
> you describe and can be reliably passed to open_by_handle_at().
>
> Yes, we should fix it but that's really orthogonal to the mount id. It
> is separately useful and we already do expose it anyway.
Put another way, name_to_handle_at(2) currently states:
Obtaining a persistent filesystem ID
The mount IDs in /proc/self/mountinfo can be reused as
filesystems are unmounted and mounted. Therefore, the mount ID
returned by name_to_handle_at() (in *mount_id) should not be
treated as a persistent identifier for the corresponding
mounted filesystem. However, an application can use the
information in the mountinfo record that corresponds to the mount
ID to derive a persistent identifier.
For example, one can use the device name in the fifth field of
the mountinfo record to search for the corresponding device UUID
via the symbolic links in /dev/disks/by-uuid. (A more
comfortable way of obtaining the UUID is to use the libblkid(3)
library.) That process can then be reversed, using the UUID to
look up the device name, and then obtaining the corre‐ sponding
mount point, in order to produce the mount_fd argument used by
open_by_handle_at().
Returning the 64bit mount id makes this race-free because we now have
statmount():
u64 mnt_id = 0;
name_to_handle_at(AT_FDCWD, "/path/to/file", &handle, &mnt_id, 0);
statmount(mnt_id);
Which gets you the device number which one can use to figure out the
uuid without ever having to open a single file (We could even expose the
UUID of the filesystem through statmount() if we wanted to.).
^ permalink raw reply
* Re: [PATCH RFC v2] fhandle: expose u64 mount id to name_to_handle_at(2)
From: Christian Brauner @ 2024-05-27 12:29 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Aleksa Sarai, Alexander Viro, Jan Kara, Chuck Lever, Jeff Layton,
Amir Goldstein, Alexander Aring, linux-fsdevel, linux-nfs,
linux-kernel, linux-api
In-Reply-To: <ZlRy7EBaV04F2UaI@infradead.org>
On Mon, May 27, 2024 at 04:47:56AM -0700, Christoph Hellwig wrote:
> On Sun, May 26, 2024 at 12:01:08PM -0700, Aleksa Sarai wrote:
> > The existing interface already provides a mount ID which is not even
> > safe without rebooting.
>
> And that seems to be a big part of the problem where the Linux by handle
> syscall API deviated from all know precedence for no good reason. NFS
> file handles which were the start of this do (and have to) encode a
> persistent file system identifier. As do the xfs handles (although they
> do the decoding in the userspace library on Linux for historic reasons),
> as do the FreeBSD equivalents to these syscalls.
>
> > An alternative would be to return something unique to the filesystem
> > superblock, but as far as I can tell there is no guarantee that every
> > Linux filesystem's fsid is sufficiently unique to act as a globally
> > unique identifier. At least with a 64-bit mount ID and statmount(2),
> > userspace can decide what information is needed to get sufficiently
> > unique information about the source filesystem.
>
> Well, every file system that supports export ops already needs a
> globally unique ID for NFS to work properly. We might not have good
> enough interfaces for that, but that shouldn't be too hard.
I see not inherent problem with exposing the 64 bit mount id through
name_to_handle_at() as we already to expose the old one anyway.
But I agree that it is useful if we had the guarantee that file handles
are unique in the way you describe. As it currently stands that doesn't
seem to be the case and userspace doesn't seem to have a way of figuring
out if the handle provided by name_to_handle_at() is indeed unique as
you describe and can be reliably passed to open_by_handle_at().
Yes, we should fix it but that's really orthogonal to the mount id. It
is separately useful and we already do expose it anyway.
^ permalink raw reply
* Re: [PATCH RFC v2] fhandle: expose u64 mount id to name_to_handle_at(2)
From: Christian Brauner @ 2024-05-27 12:22 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Aleksa Sarai, Alexander Viro, Jan Kara, Chuck Lever, Jeff Layton,
Amir Goldstein, Alexander Aring, linux-fsdevel, linux-nfs,
linux-kernel, linux-api
In-Reply-To: <ZlMADupKkN0ITgG5@infradead.org>
On Sun, May 26, 2024 at 02:25:34AM -0700, Christoph Hellwig wrote:
> On Thu, May 23, 2024 at 01:57:32PM -0700, Aleksa Sarai wrote:
> > Now that we provide a unique 64-bit mount ID interface in statx, we can
> > now provide a race-free way for name_to_handle_at(2) to provide a file
> > handle and corresponding mount without needing to worry about racing
> > with /proc/mountinfo parsing.
>
> file handles are not tied to mounts, they are tied to super_blocks,
> and they can survive reboots or (less relevant) remounts. This thus
> seems like a very confusing if not wrong interfaces.
I'm not fond of the interface as I've said on an earlier version, but
name_to_handle_at() has always exposed the mount id. IOW, this isn't
adding a new concept to the system call.
^ permalink raw reply
* Re: [PATCH RFC v2] fhandle: expose u64 mount id to name_to_handle_at(2)
From: hch @ 2024-05-27 11:49 UTC (permalink / raw)
To: Trond Myklebust
Cc: cyphar@cyphar.com, hch@infradead.org, jack@suse.cz,
brauner@kernel.org, linux-fsdevel@vger.kernel.org,
linux-api@vger.kernel.org, chuck.lever@oracle.com,
alex.aring@gmail.com, linux-kernel@vger.kernel.org,
viro@zeniv.linux.org.uk, jlayton@kernel.org, amir73il@gmail.com,
linux-nfs@vger.kernel.org
In-Reply-To: <30137c868039a3ae17f4ae74d07383099bfa4db8.camel@hammerspace.com>
On Sun, May 26, 2024 at 10:32:39PM +0000, Trond Myklebust wrote:
> I assume the reason is to give the caller a race free way to figure out
> which submount the path resolves to.
But the handle op are global to the file systems (aka super_block). It
does not matter what mount you use to access it.
Snip random things about userland NFS servers I couldn't care less
about..
^ permalink raw reply
* Re: [PATCH RFC v2] fhandle: expose u64 mount id to name_to_handle_at(2)
From: Christoph Hellwig @ 2024-05-27 11:47 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Christoph Hellwig, Alexander Viro, Christian Brauner, Jan Kara,
Chuck Lever, Jeff Layton, Amir Goldstein, Alexander Aring,
linux-fsdevel, linux-nfs, linux-kernel, linux-api
In-Reply-To: <20240526.184753-detached.length.shallow.contents-jWkMukeD7VAC@cyphar.com>
On Sun, May 26, 2024 at 12:01:08PM -0700, Aleksa Sarai wrote:
> The existing interface already provides a mount ID which is not even
> safe without rebooting.
And that seems to be a big part of the problem where the Linux by handle
syscall API deviated from all know precedence for no good reason. NFS
file handles which were the start of this do (and have to) encode a
persistent file system identifier. As do the xfs handles (although they
do the decoding in the userspace library on Linux for historic reasons),
as do the FreeBSD equivalents to these syscalls.
> An alternative would be to return something unique to the filesystem
> superblock, but as far as I can tell there is no guarantee that every
> Linux filesystem's fsid is sufficiently unique to act as a globally
> unique identifier. At least with a 64-bit mount ID and statmount(2),
> userspace can decide what information is needed to get sufficiently
> unique information about the source filesystem.
Well, every file system that supports export ops already needs a
globally unique ID for NFS to work properly. We might not have good
enough interfaces for that, but that shouldn't be too hard.
^ permalink raw reply
* Re: [PATCH RFC v2] fhandle: expose u64 mount id to name_to_handle_at(2)
From: Trond Myklebust @ 2024-05-26 22:32 UTC (permalink / raw)
To: cyphar@cyphar.com, hch@infradead.org
Cc: jack@suse.cz, brauner@kernel.org, linux-fsdevel@vger.kernel.org,
linux-api@vger.kernel.org, chuck.lever@oracle.com,
alex.aring@gmail.com, linux-kernel@vger.kernel.org,
viro@zeniv.linux.org.uk, jlayton@kernel.org, amir73il@gmail.com,
linux-nfs@vger.kernel.org
In-Reply-To: <ZlMADupKkN0ITgG5@infradead.org>
On Sun, 2024-05-26 at 02:25 -0700, Christoph Hellwig wrote:
> On Thu, May 23, 2024 at 01:57:32PM -0700, Aleksa Sarai wrote:
> > Now that we provide a unique 64-bit mount ID interface in statx, we
> > can
> > now provide a race-free way for name_to_handle_at(2) to provide a
> > file
> > handle and corresponding mount without needing to worry about
> > racing
> > with /proc/mountinfo parsing.
>
> file handles are not tied to mounts, they are tied to super_blocks,
> and they can survive reboots or (less relevant) remounts. This thus
> seems like a very confusing if not wrong interfaces.
I assume the reason is to give the caller a race free way to figure out
which submount the path resolves to. The problem is that nothing stops
another process from calling umount() before you're done parsing
/proc/mountinfo and have resolved the mount id.
If we're looking to change the API, then perhaps returning a file
descriptor might be a better alternative?
Most userland NFS servers are in any case going to follow up obtaining
the filehandle with a stat() or even a full blown open() in order to
get file attributes, set up file state, etc. By returning an open file
descriptor to the resolved file (even if it is only an O_PATH
descriptor) we could accelerate those operations in addition to solving
the umount() race.
Alternatively, just remove the path argument altogether, and require
the descriptor argument to be an O_PATH or regular open file descriptor
that resolves to the file we want to get a filehandle for. However this
would require a userland NFS server to generally do a
open_by_handle_at() to resolve the parent directory handle, then do an
openat(O_PATH) to get the file to look up, before being able to call
the name_to_handle_at() replacement.
i.e. there would be 1 extra syscall.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com
^ permalink raw reply
* Re: [PATCH RFC v2] fhandle: expose u64 mount id to name_to_handle_at(2)
From: Aleksa Sarai @ 2024-05-26 19:01 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Alexander Viro, Christian Brauner, Jan Kara, Chuck Lever,
Jeff Layton, Amir Goldstein, Alexander Aring, linux-fsdevel,
linux-nfs, linux-kernel, linux-api
In-Reply-To: <ZlMADupKkN0ITgG5@infradead.org>
[-- Attachment #1: Type: text/plain, Size: 1997 bytes --]
On 2024-05-26, Christoph Hellwig <hch@infradead.org> wrote:
> On Thu, May 23, 2024 at 01:57:32PM -0700, Aleksa Sarai wrote:
> > Now that we provide a unique 64-bit mount ID interface in statx, we can
> > now provide a race-free way for name_to_handle_at(2) to provide a file
> > handle and corresponding mount without needing to worry about racing
> > with /proc/mountinfo parsing.
>
> file handles are not tied to mounts, they are tied to super_blocks,
> and they can survive reboots or (less relevant) remounts. This thus
> seems like a very confusing if not wrong interfaces.
The existing interface already provides a mount ID which is not even
safe without rebooting.
The purpose of the mount ID (in the existing API) is to allow userspace
to make sure they know which filesystem mount the file handle came from
so they can store the relevant information (fsid, etc) to make sure they
know which superblock the mount came from when it comes to doing
open_by_handle_at(2). However, there is a race between getting the mount
ID from name_to_handle_at() and parsing /proc/self/mountinfo, which
means that userspace cannot ever be sure that they know the correct
mount the file handle came from (the man page for open_by_handle_at
mentions this issue explicitly).
Getting the ability to get a 64-bit mount ID would allow userspace to
use statmount(2) directly, avoiding this race. You're quite right that
obviously you wouldn't want to just store the mount ID.
An alternative would be to return something unique to the filesystem
superblock, but as far as I can tell there is no guarantee that every
Linux filesystem's fsid is sufficiently unique to act as a globally
unique identifier. At least with a 64-bit mount ID and statmount(2),
userspace can decide what information is needed to get sufficiently
unique information about the source filesystem.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH RFC v2] fhandle: expose u64 mount id to name_to_handle_at(2)
From: Christoph Hellwig @ 2024-05-26 9:25 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Alexander Viro, Christian Brauner, Jan Kara, Chuck Lever,
Jeff Layton, Amir Goldstein, Alexander Aring, linux-fsdevel,
linux-nfs, linux-kernel, linux-api
In-Reply-To: <20240523-exportfs-u64-mount-id-v2-1-f9f959f17eb1@cyphar.com>
On Thu, May 23, 2024 at 01:57:32PM -0700, Aleksa Sarai wrote:
> Now that we provide a unique 64-bit mount ID interface in statx, we can
> now provide a race-free way for name_to_handle_at(2) to provide a file
> handle and corresponding mount without needing to worry about racing
> with /proc/mountinfo parsing.
file handles are not tied to mounts, they are tied to super_blocks,
and they can survive reboots or (less relevant) remounts. This thus
seems like a very confusing if not wrong interfaces.
^ permalink raw reply
* Re: [PATCH v2] xfs_io: make MADV_SOFT_OFFLINE conditional
From: Bastian Germann @ 2024-05-25 11:25 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-xfs, Darrick J . Wong, Thomas Bogendoerfer, linux-mips,
linux-api
In-Reply-To: <ZlBNPDomQpTfAmt2@infradead.org>
Am 24.05.24 um 10:18 schrieb Christoph Hellwig:
>> +#ifdef MADV_SOFT_OFFLINE
>
> Can you add a comment here that this is missing on mips as of Linux
> 6.9?
Sure.
> Also adding the mips maintainer so he can add it, as missing a madvise
> random feature on a random architecture looks like a bug that should be
> fixed.
There are more of this class of bugs which f82b77462b ("tools include:
Add mman macros needed by perf for all arch") presents.
^ permalink raw reply
* Re: [PATCH v2 1/1] futex: Add FUTEX_SPIN operation
From: Mateusz Guzik @ 2024-05-25 9:36 UTC (permalink / raw)
To: André Almeida
Cc: Mathieu Desnoyers, Peter Zijlstra, linux-kernel, Thomas Gleixner,
Paul E . McKenney, Boqun Feng, H . Peter Anvin, Paul Turner,
linux-api, Christian Brauner, Florian Weimer, David.Laight,
carlos, Peter Oskolkov, Alexander Mikhalitsyn, Chris Kennelly,
Ingo Molnar, Darren Hart, Davidlohr Bueso, libc-alpha,
Steven Rostedt, Jonathan Corbet, Noah Goldstein,
Daniel Colascione, longman, kernel-dev
In-Reply-To: <20240523200704.281514-2-andrealmeid@igalia.com>
On Thu, May 23, 2024 at 05:07:04PM -0300, André Almeida wrote:
> Add a new mode for futex wait, the futex spin.
>
I'll note that currently individuals who want to reduce performance
degradation due to contention in userspace either handroll some
speculative spinning + futex usage or straight up only spin, which btw
is a massive pet peeve of mine.
With this in mind the real reference point justifying this mechanism (or
showing it not being good enough to warrant inclusion) is a case where
all participating threads are on cpu and spinning is done in userspace
(in a sensible manner). Of course there is more than one way to spin,
but that is a minor point in the grand scheme of things.
The benchmark you linked in the cover letter definitely would use some
improvement, it was mentioned elsewhere that some of the time is spent
just waiting for threads to be created. The usual way to handle this is
by parking everyone on a barrier before the measurement starts. You can
use will-it-scale as an example how to do it, or better yet plug your
stuff into it as testcases instead. It even already has some contested
pthread mutex benchmarks.
So I would say proper test matrix would include pthread mutexes as is,
FUTEX2_SPIN and mere spinning (just load + cpu_relax, I can ship you
with sample code for will-it-scale if you want).
Even the best possible variant of FUTEX2_SPIN has to do worse than mere
spinning in userspace -- in a HT setup it hinders execution by coming
here instead of just cpu_relax-ing, and that aside it may be introducing
dead time where the lock is free to use by the cpu is busy going in and
out of the kernel. The question is if it is going to do well enough to
persuade people to not bother with their own hacks, which it plausibly
will.
> Given the FUTEX2_SPIN flag, parse the futex value as the TID of the lock
> owner. Then, before going to the normal wait path, spins while the lock
> owner is running in a different CPU, to avoid the whole context switch
> operation and to quickly return to userspace. If the lock owner is not
> running, just sleep as the normal futex wait path.
>
Syscall costs are so high that by the time you get to this code chances
are decent the owner already released the lock and it is now being held
by someone else, also see below.
> +static int futex_spin(struct futex_hash_bucket *hb, struct futex_q *q,
> + struct hrtimer_sleeper *timeout, u32 uval)
> +{
> + struct task_struct *p;
> + pid_t pid = uval & FUTEX_TID_MASK;
> +
> + p = find_get_task_by_vpid(pid);
> +
In order to even get here all prospective spinners have to serialize on
a spinlock. Then they further dirty a process by grabbing a ref on it.
This seriously hinders the mechanism but is avoidable.
> + /* no task found, maybe it already exited */
> + if (!p) {
> + futex_q_unlock(hb);
> + return -EAGAIN;
> + }
> +
> + /* can't spin in a kernel task */
> + if (unlikely(p->flags & PF_KTHREAD)) {
> + put_task_struct(p);
> + futex_q_unlock(hb);
> + return -EPERM;
> + }
> +
> + futex_queue(q, hb);
> +
> + if (timeout)
> + hrtimer_sleeper_start_expires(timeout, HRTIMER_MODE_ABS);
> +
> + while (1) {
> + if (likely(!plist_node_empty(&q->list))) {
> + if (timeout && !timeout->task)
> + goto exit;
> +
> + if (task_on_cpu(p)) {
> + /* spin */
cpu_relax() is the thing to do here, it was already mentioned by someone
else.
My comment is that as mentioned above the lock owner by now may be
different. So to my reading this spins on the lock as long as p is
running, which may not even be holding it at the moment. Worse, by now
the p task may also be spinning in this very place.
That is to say I don't believe passing TID as an argument to the
syscall is a viable approach.
Instead this code will have to read the futex value on its own every
time (with a special accessor which does not allow for page faults) and
do the find_get_task_by_vpid + task_on_cpu combo under RCU (no refing of
the proc).
The queue locking would also be dodged, except for the case where the
code decides to go off cpu.
Short of something like that imho this has no hopes of competing with
userspace spinning.
^ permalink raw reply
* Re: [PATCH v2] xfs_io: make MADV_SOFT_OFFLINE conditional
From: Christoph Hellwig @ 2024-05-24 8:18 UTC (permalink / raw)
To: Bastian Germann
Cc: linux-xfs, Darrick J . Wong, Thomas Bogendoerfer, linux-mips,
linux-api
In-Reply-To: <20240522223748.9986-1-bage@debian.org>
> +#ifdef MADV_SOFT_OFFLINE
Can you add a comment here that this is missing on mips as of Linux
6.9?
Also adding the mips maintainer so he can add it, as missing a madvise
random feature on a random architecture looks like a bug that should be
fixed.
^ permalink raw reply
* RE: [PATCH v2 1/1] futex: Add FUTEX_SPIN operation
From: David Laight @ 2024-05-24 7:52 UTC (permalink / raw)
To: 'André Almeida', Mathieu Desnoyers, Peter Zijlstra
Cc: linux-kernel@vger.kernel.org, Thomas Gleixner, Paul E . McKenney,
Boqun Feng, H . Peter Anvin, Paul Turner,
linux-api@vger.kernel.org, Christian Brauner, Florian Weimer,
carlos@redhat.com, Peter Oskolkov, Alexander Mikhalitsyn,
Chris Kennelly, Ingo Molnar, Darren Hart, Davidlohr Bueso,
libc-alpha@sourceware.org, Steven Rostedt, Jonathan Corbet,
Noah Goldstein, Daniel Colascione, longman@redhat.com,
kernel-dev@igalia.com
In-Reply-To: <20240523200704.281514-2-andrealmeid@igalia.com>
From: André Almeida
> Sent: 23 May 2024 21:07
>
> Add a new mode for futex wait, the futex spin.
>
> Given the FUTEX2_SPIN flag, parse the futex value as the TID of the lock
> owner. Then, before going to the normal wait path, spins while the lock
> owner is running in a different CPU, to avoid the whole context switch
> operation and to quickly return to userspace. If the lock owner is not
> running, just sleep as the normal futex wait path.
>
> The user value is masked with FUTEX_TID_MASK, to allow some bits for
> future use.
>
> The check for the owner to be running or not is important to avoid
> spinning for something that won't be released quickly. Userspace is
> responsible on providing the proper TID, the kernel does a basic check.
The kernel needs to do something to stop a user-process spinning in-kernel
indefinitely.
...
> +static inline bool task_on_cpu(struct task_struct *p)
> +{
> +#ifdef CONFIG_SMP
> + return !!(p->on_cpu);
> +#else
> + return false;
> +#endif
> +}
I suspect that isn't going to work in a VM where the entire 'cpu'
can be sleeping.
This is similar to the (I don't think works properly) check
in the 'osq' (optimistic spin queue) used when waiting for
the thread spinning on a mutex/rwlock to change state.
IIRC that code also checks whether the current thread should
be pre-empted by a higher priority process.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH RFC v2] fhandle: expose u64 mount id to name_to_handle_at(2)
From: Amir Goldstein @ 2024-05-24 4:58 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Alexander Viro, Christian Brauner, Jan Kara, Chuck Lever,
Jeff Layton, Alexander Aring, linux-fsdevel, linux-nfs,
linux-kernel, linux-api
In-Reply-To: <20240523-exportfs-u64-mount-id-v2-1-f9f959f17eb1@cyphar.com>
On Thu, May 23, 2024 at 11:57 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> Now that we provide a unique 64-bit mount ID interface in statx, we can
> now provide a race-free way for name_to_handle_at(2) to provide a file
> handle and corresponding mount without needing to worry about racing
> with /proc/mountinfo parsing.
>
> While this is not necessary if you are using AT_EMPTY_PATH and don't
> care about an extra statx(2) call, users that pass full paths into
> name_to_handle_at(2) need to know which mount the file handle comes from
> (to make sure they don't try to open_by_handle_at a file handle from a
> different filesystem) and switching to AT_EMPTY_PATH would require
> allocating a file for every name_to_handle_at(2) call, turning
>
> err = name_to_handle_at(-EBADF, "/foo/bar/baz", &handle, &mntid,
> AT_HANDLE_MNT_ID_UNIQUE);
>
> into
>
> int fd = openat(-EBADF, "/foo/bar/baz", O_PATH | O_CLOEXEC);
> err1 = name_to_handle_at(fd, "", &handle, &unused_mntid, AT_EMPTY_PATH);
> err2 = statx(fd, "", AT_EMPTY_PATH, STATX_MNT_ID_UNIQUE, &statxbuf);
> mntid = statxbuf.stx_mnt_id;
> close(fd);
>
> Unlike AT_HANDLE_FID, as per Amir's suggestion, AT_HANDLE_MNT_ID_UNIQUE
> uses a new AT_* bit from the historically-unused 0xFF range (which we
> now define as being the "per-syscall" range for AT_* bits).
>
Sorry for nit picking, but I think that "Unlike AT_HANDLE_FID,..." is confusing
in this statement.
AT_HANDLE_FID is using a bit that was already effectively allocated for a
"per-syscall" range.
I don't think that mentioning AT_HANDLE_FID adds any clarity to the statement
so better drop it?
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> ---
> Changes in v2:
> - Fixed a few minor compiler warnings and a buggy copy_to_user() check.
> - Rename AT_HANDLE_UNIQUE_MOUNT_ID -> AT_HANDLE_MNT_ID_UNIQUE to match statx.
> - Switched to using an AT_* bit from 0xFF and defining that range as
> being "per-syscall" for future usage.
> - Sync tools/ copy of <linux/fcntl.h> to include changes.
> - v1: <https://lore.kernel.org/r/20240520-exportfs-u64-mount-id-v1-1-f55fd9215b8e@cyphar.com>
> ---
> fs/fhandle.c | 29 ++++++++++++++++++++++-------
> include/linux/syscalls.h | 2 +-
> include/uapi/linux/fcntl.h | 28 +++++++++++++++++++++-------
> tools/include/uapi/linux/fcntl.h | 28 +++++++++++++++++++++-------
> 4 files changed, 65 insertions(+), 22 deletions(-)
>
[...]
> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> index c0bcc185fa48..9ed9d65842c1 100644
> --- a/include/uapi/linux/fcntl.h
> +++ b/include/uapi/linux/fcntl.h
> @@ -87,22 +87,24 @@
> #define DN_ATTRIB 0x00000020 /* File changed attibutes */
> #define DN_MULTISHOT 0x80000000 /* Don't remove notifier */
>
> +#define AT_FDCWD -100 /* Special value used to indicate
> + openat should use the current
> + working directory. */
(more nit picking)
If you are changing this line, please at least add a new line,
this is a different namespace :-/
and perhaps change it to "Special value of dirfd argument..."
Also, better leave a comment here to discourage allocation from this range:
+ /* Reserved for per-syscall flags 0xff */
> +#define AT_SYMLINK_NOFOLLOW 0x100 /* Do not follow symbolic links. */
> +
> /*
> - * The constants AT_REMOVEDIR and AT_EACCESS have the same value. AT_EACCESS is
> - * meaningful only to faccessat, while AT_REMOVEDIR is meaningful only to
> + * The constants AT_REMOVEDIR and AT_EACCESS have the same value. AT_EACCESS
> + * is meaningful only to faccessat, while AT_REMOVEDIR is meaningful only to
> * unlinkat. The two functions do completely different things and therefore,
> * the flags can be allowed to overlap. For example, passing AT_REMOVEDIR to
> * faccessat would be undefined behavior and thus treating it equivalent to
> * AT_EACCESS is valid undefined behavior.
> */
If you are going to add this churn in this patch, please do it otherwise.
It does not make sense to have this long explanation about pre-syscall
AT_* flags in a different location from the comment you added about
"All new purely-syscall-specific AT_* flags.."
if this explanation is needed at all, it should be after the new comment
as an example.
> -#define AT_FDCWD -100 /* Special value used to indicate
> - openat should use the current
> - working directory. */
> -#define AT_SYMLINK_NOFOLLOW 0x100 /* Do not follow symbolic links. */
> #define AT_EACCESS 0x200 /* Test access permitted for
> effective IDs, not real IDs. */
> #define AT_REMOVEDIR 0x200 /* Remove directory instead of
> unlinking file. */
I really prefer to move those to the per-syscall section
right next to AT_HANDLE_FID and leave a comment here:
/* Reserved for per-syscall flags 0x200 */
> +
> #define AT_SYMLINK_FOLLOW 0x400 /* Follow symbolic links. */
> #define AT_NO_AUTOMOUNT 0x800 /* Suppress terminal automount traversal */
> #define AT_EMPTY_PATH 0x1000 /* Allow empty relative pathname */
> @@ -114,10 +116,22 @@
>
> #define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */
>
> -/* Flags for name_to_handle_at(2). We reuse AT_ flag space to save bits... */
> +/*
> + * All new purely-syscall-specific AT_* flags should consider using bits from
> + * 0xFF, but the bits used by RENAME_* (0x7) should be avoided in case users
> + * decide to pass AT_* flags to renameat2() by accident.
Sorry, but I find the use of my renameat2() example a bit confusing
in this sentence.
If you mention it at all, please use "For example, the bits used by..."
but I think it is more important to say "...should consider re-using bits
already used by other per-syscalls flags".
> These flag bits are
> + * free for re-use by other syscall's syscall-specific flags without worry.
> + */
> +
> +/*
> + * Flags for name_to_handle_at(2). To save AT_ flag space we re-use the
> + * AT_EACCESS/AT_REMOVEDIR bit for AT_HANDLE_FID.
> + */
AT_EACCESS/AT_REMOVEDIR/AT_HANDLE_FID have exact same status,
so instead of this asymmetric comment:
+/* Flags for faccessat(2) */
+#define AT_EACCESS 0x200 /* Test access permitted for
+ effective IDs, not real IDs. */
+/* Flags for unlinkat(2) */
+#define AT_REMOVEDIR 0x200 /* Remove directory instead of
+ unlinking file. */
+/* Flags for name_to_handle_at(2) */
+#define AT_HANDLE_FID 0x200 /* file handle is needed to
compare object identity and may not
be usable to open_by_handle_at(2) */
> +#define AT_HANDLE_MNT_ID_UNIQUE 0x80 /* return the u64 unique mount id */
IDGI, I think we may have been miscommunicating :-/
If 0x7 range is to be avoided for generic AT_ flags, then it *should* be used
for new per-syscall flags such as this one.
The reservation of 0xff is not a strong guarantee.
As long as people re-use new per-syscalls flags efficiently, we could
decide to reclaim some of this space for generic AT_ flags in the future
if it is needed.
I know most of the mess was here before your patch, but I think
it got to a point where we must put a little order before introducing
the new per-syscall flag.
Thanks,
Amir.
^ permalink raw reply
* [PATCH RFC v2] fhandle: expose u64 mount id to name_to_handle_at(2)
From: Aleksa Sarai @ 2024-05-23 20:57 UTC (permalink / raw)
To: Alexander Viro, Christian Brauner, Jan Kara, Chuck Lever,
Jeff Layton, Amir Goldstein, Alexander Aring
Cc: linux-fsdevel, linux-nfs, linux-kernel, linux-api, Aleksa Sarai
Now that we provide a unique 64-bit mount ID interface in statx, we can
now provide a race-free way for name_to_handle_at(2) to provide a file
handle and corresponding mount without needing to worry about racing
with /proc/mountinfo parsing.
While this is not necessary if you are using AT_EMPTY_PATH and don't
care about an extra statx(2) call, users that pass full paths into
name_to_handle_at(2) need to know which mount the file handle comes from
(to make sure they don't try to open_by_handle_at a file handle from a
different filesystem) and switching to AT_EMPTY_PATH would require
allocating a file for every name_to_handle_at(2) call, turning
err = name_to_handle_at(-EBADF, "/foo/bar/baz", &handle, &mntid,
AT_HANDLE_MNT_ID_UNIQUE);
into
int fd = openat(-EBADF, "/foo/bar/baz", O_PATH | O_CLOEXEC);
err1 = name_to_handle_at(fd, "", &handle, &unused_mntid, AT_EMPTY_PATH);
err2 = statx(fd, "", AT_EMPTY_PATH, STATX_MNT_ID_UNIQUE, &statxbuf);
mntid = statxbuf.stx_mnt_id;
close(fd);
Unlike AT_HANDLE_FID, as per Amir's suggestion, AT_HANDLE_MNT_ID_UNIQUE
uses a new AT_* bit from the historically-unused 0xFF range (which we
now define as being the "per-syscall" range for AT_* bits).
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
Changes in v2:
- Fixed a few minor compiler warnings and a buggy copy_to_user() check.
- Rename AT_HANDLE_UNIQUE_MOUNT_ID -> AT_HANDLE_MNT_ID_UNIQUE to match statx.
- Switched to using an AT_* bit from 0xFF and defining that range as
being "per-syscall" for future usage.
- Sync tools/ copy of <linux/fcntl.h> to include changes.
- v1: <https://lore.kernel.org/r/20240520-exportfs-u64-mount-id-v1-1-f55fd9215b8e@cyphar.com>
---
fs/fhandle.c | 29 ++++++++++++++++++++++-------
include/linux/syscalls.h | 2 +-
include/uapi/linux/fcntl.h | 28 +++++++++++++++++++++-------
tools/include/uapi/linux/fcntl.h | 28 +++++++++++++++++++++-------
4 files changed, 65 insertions(+), 22 deletions(-)
diff --git a/fs/fhandle.c b/fs/fhandle.c
index 8a7f86c2139a..c6e90161b900 100644
--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -16,7 +16,8 @@
static long do_sys_name_to_handle(const struct path *path,
struct file_handle __user *ufh,
- int __user *mnt_id, int fh_flags)
+ void __user *mnt_id, bool unique_mntid,
+ int fh_flags)
{
long retval;
struct file_handle f_handle;
@@ -69,9 +70,19 @@ static long do_sys_name_to_handle(const struct path *path,
} else
retval = 0;
/* copy the mount id */
- if (put_user(real_mount(path->mnt)->mnt_id, mnt_id) ||
- copy_to_user(ufh, handle,
- struct_size(handle, f_handle, handle_bytes)))
+ if (unique_mntid) {
+ if (put_user(real_mount(path->mnt)->mnt_id_unique,
+ (u64 __user *) mnt_id))
+ retval = -EFAULT;
+ } else {
+ if (put_user(real_mount(path->mnt)->mnt_id,
+ (int __user *) mnt_id))
+ retval = -EFAULT;
+ }
+ /* copy the handle */
+ if (retval != -EFAULT &&
+ copy_to_user(ufh, handle,
+ struct_size(handle, f_handle, handle_bytes)))
retval = -EFAULT;
kfree(handle);
return retval;
@@ -83,6 +94,7 @@ static long do_sys_name_to_handle(const struct path *path,
* @name: name that should be converted to handle.
* @handle: resulting file handle
* @mnt_id: mount id of the file system containing the file
+ * (u64 if AT_HANDLE_MNT_ID_UNIQUE, otherwise int)
* @flag: flag value to indicate whether to follow symlink or not
* and whether a decodable file handle is required.
*
@@ -92,7 +104,7 @@ static long do_sys_name_to_handle(const struct path *path,
* value required.
*/
SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
- struct file_handle __user *, handle, int __user *, mnt_id,
+ struct file_handle __user *, handle, void __user *, mnt_id,
int, flag)
{
struct path path;
@@ -100,7 +112,8 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
int fh_flags;
int err;
- if (flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH | AT_HANDLE_FID))
+ if (flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH | AT_HANDLE_FID |
+ AT_HANDLE_MNT_ID_UNIQUE))
return -EINVAL;
lookup_flags = (flag & AT_SYMLINK_FOLLOW) ? LOOKUP_FOLLOW : 0;
@@ -109,7 +122,9 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
lookup_flags |= LOOKUP_EMPTY;
err = user_path_at(dfd, name, lookup_flags, &path);
if (!err) {
- err = do_sys_name_to_handle(&path, handle, mnt_id, fh_flags);
+ err = do_sys_name_to_handle(&path, handle, mnt_id,
+ flag & AT_HANDLE_MNT_ID_UNIQUE,
+ fh_flags);
path_put(&path);
}
return err;
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index e619ac10cd23..99c5a783a01e 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -863,7 +863,7 @@ asmlinkage long sys_fanotify_mark(int fanotify_fd, unsigned int flags,
const char __user *pathname);
asmlinkage long sys_name_to_handle_at(int dfd, const char __user *name,
struct file_handle __user *handle,
- int __user *mnt_id, int flag);
+ void __user *mnt_id, int flag);
asmlinkage long sys_open_by_handle_at(int mountdirfd,
struct file_handle __user *handle,
int flags);
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index c0bcc185fa48..9ed9d65842c1 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -87,22 +87,24 @@
#define DN_ATTRIB 0x00000020 /* File changed attibutes */
#define DN_MULTISHOT 0x80000000 /* Don't remove notifier */
+#define AT_FDCWD -100 /* Special value used to indicate
+ openat should use the current
+ working directory. */
+#define AT_SYMLINK_NOFOLLOW 0x100 /* Do not follow symbolic links. */
+
/*
- * The constants AT_REMOVEDIR and AT_EACCESS have the same value. AT_EACCESS is
- * meaningful only to faccessat, while AT_REMOVEDIR is meaningful only to
+ * The constants AT_REMOVEDIR and AT_EACCESS have the same value. AT_EACCESS
+ * is meaningful only to faccessat, while AT_REMOVEDIR is meaningful only to
* unlinkat. The two functions do completely different things and therefore,
* the flags can be allowed to overlap. For example, passing AT_REMOVEDIR to
* faccessat would be undefined behavior and thus treating it equivalent to
* AT_EACCESS is valid undefined behavior.
*/
-#define AT_FDCWD -100 /* Special value used to indicate
- openat should use the current
- working directory. */
-#define AT_SYMLINK_NOFOLLOW 0x100 /* Do not follow symbolic links. */
#define AT_EACCESS 0x200 /* Test access permitted for
effective IDs, not real IDs. */
#define AT_REMOVEDIR 0x200 /* Remove directory instead of
unlinking file. */
+
#define AT_SYMLINK_FOLLOW 0x400 /* Follow symbolic links. */
#define AT_NO_AUTOMOUNT 0x800 /* Suppress terminal automount traversal */
#define AT_EMPTY_PATH 0x1000 /* Allow empty relative pathname */
@@ -114,10 +116,22 @@
#define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */
-/* Flags for name_to_handle_at(2). We reuse AT_ flag space to save bits... */
+/*
+ * All new purely-syscall-specific AT_* flags should consider using bits from
+ * 0xFF, but the bits used by RENAME_* (0x7) should be avoided in case users
+ * decide to pass AT_* flags to renameat2() by accident. These flag bits are
+ * free for re-use by other syscall's syscall-specific flags without worry.
+ */
+
+/*
+ * Flags for name_to_handle_at(2). To save AT_ flag space we re-use the
+ * AT_EACCESS/AT_REMOVEDIR bit for AT_HANDLE_FID.
+ */
#define AT_HANDLE_FID AT_REMOVEDIR /* file handle is needed to
compare object identity and may not
be usable to open_by_handle_at(2) */
+#define AT_HANDLE_MNT_ID_UNIQUE 0x80 /* return the u64 unique mount id */
+
#if defined(__KERNEL__)
#define AT_GETATTR_NOSEC 0x80000000
#endif
diff --git a/tools/include/uapi/linux/fcntl.h b/tools/include/uapi/linux/fcntl.h
index 282e90aeb163..f671312ca481 100644
--- a/tools/include/uapi/linux/fcntl.h
+++ b/tools/include/uapi/linux/fcntl.h
@@ -85,22 +85,24 @@
#define DN_ATTRIB 0x00000020 /* File changed attibutes */
#define DN_MULTISHOT 0x80000000 /* Don't remove notifier */
+#define AT_FDCWD -100 /* Special value used to indicate
+ openat should use the current
+ working directory. */
+#define AT_SYMLINK_NOFOLLOW 0x100 /* Do not follow symbolic links. */
+
/*
- * The constants AT_REMOVEDIR and AT_EACCESS have the same value. AT_EACCESS is
- * meaningful only to faccessat, while AT_REMOVEDIR is meaningful only to
+ * The constants AT_REMOVEDIR and AT_EACCESS have the same value. AT_EACCESS
+ * is meaningful only to faccessat, while AT_REMOVEDIR is meaningful only to
* unlinkat. The two functions do completely different things and therefore,
* the flags can be allowed to overlap. For example, passing AT_REMOVEDIR to
* faccessat would be undefined behavior and thus treating it equivalent to
* AT_EACCESS is valid undefined behavior.
*/
-#define AT_FDCWD -100 /* Special value used to indicate
- openat should use the current
- working directory. */
-#define AT_SYMLINK_NOFOLLOW 0x100 /* Do not follow symbolic links. */
#define AT_EACCESS 0x200 /* Test access permitted for
effective IDs, not real IDs. */
#define AT_REMOVEDIR 0x200 /* Remove directory instead of
unlinking file. */
+
#define AT_SYMLINK_FOLLOW 0x400 /* Follow symbolic links. */
#define AT_NO_AUTOMOUNT 0x800 /* Suppress terminal automount traversal */
#define AT_EMPTY_PATH 0x1000 /* Allow empty relative pathname */
@@ -112,10 +114,22 @@
#define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */
-/* Flags for name_to_handle_at(2). We reuse AT_ flag space to save bits... */
+/*
+ * All new purely-syscall-specific AT_* flags should consider using bits from
+ * 0xFF, but the bits used by RENAME_* (0x7) should be avoided in case users
+ * decide to pass AT_* flags to renameat2() by accident. These flag bits are
+ * free for re-use by other syscall's syscall-specific flags without worry.
+ */
+
+/*
+ * Flags for name_to_handle_at(2). To save AT_ flag space we re-use the
+ * AT_EACCESS/AT_REMOVEDIR bit for AT_HANDLE_FID.
+ */
#define AT_HANDLE_FID AT_REMOVEDIR /* file handle is needed to
compare object identity and may not
be usable to open_by_handle_at(2) */
+#define AT_HANDLE_MNT_ID_UNIQUE 0x80 /* returned mount id is the u64 unique mount id */
+
#if defined(__KERNEL__)
#define AT_GETATTR_NOSEC 0x80000000
#endif
---
base-commit: 584bbf439d0fa83d728ec49f3a38c581bdc828b4
change-id: 20240515-exportfs-u64-mount-id-9ebb5c58b53c
Best regards,
--
Aleksa Sarai <cyphar@cyphar.com>
^ permalink raw reply related
* Re: [PATCH v2 0/1] Add FUTEX_SPIN operation
From: Mathieu Desnoyers @ 2024-05-23 20:39 UTC (permalink / raw)
To: André Almeida, Peter Zijlstra
Cc: linux-kernel, Thomas Gleixner, Paul E . McKenney, Boqun Feng,
H . Peter Anvin, Paul Turner, linux-api, Christian Brauner,
Florian Weimer, David.Laight, carlos, Peter Oskolkov,
Alexander Mikhalitsyn, Chris Kennelly, Ingo Molnar, Darren Hart,
Davidlohr Bueso, libc-alpha, Steven Rostedt, Jonathan Corbet,
Noah Goldstein, longman, kernel-dev
In-Reply-To: <20240523200704.281514-1-andrealmeid@igalia.com>
On 2024-05-23 16:07, André Almeida wrote:
> Hi,
>
> In the last LPC, Mathieu Desnoyers and I presented[0] a proposal to extend the
> rseq interface to be able to implement spin locks in userspace correctly. Thomas
> Gleixner agreed that this is something that Linux could improve, but asked for
> an alternative proposal first: a futex operation that allows to spin a user
> lock inside the kernel. This patchset implements a prototype of this idea for
> further discussion.
>
> With FUTEX2_SPIN flag set during a futex_wait(), the futex value is expected to
> be the TID of the lock owner. Then, the kernel gets the task_struct of the
> corresponding TID, and checks if it's running. It spins until the futex
> is awaken, the task is scheduled out or if a timeout happens. If the lock owner
> is scheduled out at any time, then the syscall follows the normal path of
> sleeping as usual. The user input is masked with FUTEX_TID_MASK so we have some
> bits to play.
>
> If the futex is awaken and we are spinning, we can return to userspace quickly,
> avoid the scheduling out and in again to wake from a futex_wait(), thus
> speeding up the wait operation. The user input is masked with FUTEX_TID_MASK so
> we have some bits to play.
>
> Christian Brauner suggested using pidfd to avoid race conditions, and I will
> implement that in the next patch iteration. I benchmarked the implementation
> measuring the time required to wait for a futex for a simple loop using the code
> at [2]. In my setup, the total wait time for 1000 futexes using the spin method
> was almost 10% lower than just using the normal futex wait:
>
> Testing with FUTEX2_SPIN | FUTEX_WAIT
> Total wait time: 8650089 usecs
>
> Testing with FUTEX_WAIT
> Total wait time: 9447291 usecs
>
> However, as I played with how long the lock owner would be busy, the
> benchmark results of spinning vs no spinning would match, showing that the
> spinning will be effective for some specific scheduling scenarios, but depending
> on the wait time, there's no big difference either spinning or not.
>
> [0] https://lpc.events/event/17/contributions/1481/
>
> You can find a small snippet to play with this interface here:
>
> [1] https://gist.github.com/andrealmeid/f0b8c93a3c7a5c50458247c47f7078e1
What exactly are you trying to benchmark here ? I've looked at this toy
program, and I suspect that most of the delay you observe is due to
initial scheduling of a newly cloned thread, because this is what is
repeatedly being done in the delay you measure.
I would recommend to change this benchmark program to measure something
meaningful, e.g.:
- N threads repeatedly contending on a lock, until a "stop" flag is set,
- run for "duration" seconds, after which main() sets a "stop" flag.
- delay loop of "work_delay" us within the lock critical section,
- delay loop of "inactive_delay" us between locking attempts,
- measure the time it takes to grab the lock, report stats on this,
- measure the total number of operations done within the given
"duration".
- report statistics on the number of operations per thread to see
the impact on fairness,
The run the program with the following constraints:
- Pin one thread per core, with nb thread <= nb cores. This should
be a best case scenario for spinning.
- Pin all threads to a single core. when nb threads > nb cores, this
should be the worse scenario for spinning.
- Groups things between those two extremes to see how things evolve.
I would not be surprised that if you measure relevant delays, you will
observe much better speedups than what you currently have.
Thanks,
Mathieu
>
> Changelog:
>
> v1: - s/PID/TID
> - masked user input with FUTEX_TID_MASK
> - add benchmark tool to the cover letter
> - dropped debug prints
> - added missing put_task_struct()
>
> André Almeida (1):
> futex: Add FUTEX_SPIN operation
>
> include/uapi/linux/futex.h | 2 +-
> kernel/futex/futex.h | 6 ++-
> kernel/futex/waitwake.c | 78 +++++++++++++++++++++++++++++++++++++-
> 3 files changed, 82 insertions(+), 4 deletions(-)
>
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
^ permalink raw reply
* Re: [PATCH v2 1/1] futex: Add FUTEX_SPIN operation
From: Mathieu Desnoyers @ 2024-05-23 20:20 UTC (permalink / raw)
To: André Almeida, Peter Zijlstra
Cc: linux-kernel, Thomas Gleixner, Paul E . McKenney, Boqun Feng,
H . Peter Anvin, Paul Turner, linux-api, Christian Brauner,
Florian Weimer, David.Laight, carlos, Peter Oskolkov,
Alexander Mikhalitsyn, Chris Kennelly, Ingo Molnar, Darren Hart,
Davidlohr Bueso, libc-alpha, Steven Rostedt, Jonathan Corbet,
Noah Goldstein, Daniel Colascione, longman, kernel-dev
In-Reply-To: <20240523200704.281514-2-andrealmeid@igalia.com>
On 2024-05-23 16:07, André Almeida wrote:
> Add a new mode for futex wait, the futex spin.
>
> Given the FUTEX2_SPIN flag, parse the futex value as the TID of the lock
> owner. Then, before going to the normal wait path, spins while the lock
> owner is running in a different CPU, to avoid the whole context switch
> operation and to quickly return to userspace. If the lock owner is not
> running, just sleep as the normal futex wait path.
>
> The user value is masked with FUTEX_TID_MASK, to allow some bits for
> future use.
>
> The check for the owner to be running or not is important to avoid
> spinning for something that won't be released quickly. Userspace is
> responsible on providing the proper TID, the kernel does a basic check.
>
> Signed-off-by: André Almeida <andrealmeid@igalia.com>
> ---
[...]
> +
> +static int futex_spin(struct futex_hash_bucket *hb, struct futex_q *q,
> + struct hrtimer_sleeper *timeout, u32 uval)
> +{
> + struct task_struct *p;
> + pid_t pid = uval & FUTEX_TID_MASK;
> +
> + p = find_get_task_by_vpid(pid);
> +
> + /* no task found, maybe it already exited */
> + if (!p) {
> + futex_q_unlock(hb);
> + return -EAGAIN;
> + }
> +
> + /* can't spin in a kernel task */
> + if (unlikely(p->flags & PF_KTHREAD)) {
> + put_task_struct(p);
> + futex_q_unlock(hb);
> + return -EPERM;
> + }
> +
> + futex_queue(q, hb);
> +
> + if (timeout)
> + hrtimer_sleeper_start_expires(timeout, HRTIMER_MODE_ABS);
> +
> + while (1) {
Infinite loops in other kernel/futex/ files appear to use "for (;;) {"
instead.
> + if (likely(!plist_node_empty(&q->list))) {
> + if (timeout && !timeout->task)
> + goto exit;
> +
> + if (task_on_cpu(p)) {
> + /* spin */
You may want to add a "cpu_relax();" here to lessen the
power consumption of this busy-loop.
> + continue;
> + } else {
> + /* task is not running, sleep */
> + break;
> + }
> + } else {
> + goto exit;
> + }
> + }
> +
> + /* spinning didn't work, go to the normal path */
> + set_current_state(TASK_INTERRUPTIBLE|TASK_FREEZABLE);
I wonder if flipping the order between:
set_current_state(TASK_INTERRUPTIBLE|TASK_FREEZABLE);
futex_queue(q, hb);
as done in futex_wait_queue() and what is done here matters ?
Does it introduce a race where a wakeup could be missed ?
If it's an issue, then setting the current state could be done
before invoking futex_queue(), and whenever the spin exits,
set it back to TASK_RUNNING.
> +
> + if (likely(!plist_node_empty(&q->list))) {
> + if (!timeout || timeout->task)
> + schedule();
> + }
> +
> + __set_current_state(TASK_RUNNING);
> +
> +exit:
> + put_task_struct(p);
> + return 0;
> +}
> +
> /**
> * futex_unqueue_multiple - Remove various futexes from their hash bucket
> * @v: The list of futexes to unqueue
> @@ -665,8 +732,15 @@ int __futex_wait(u32 __user *uaddr, unsigned int flags, u32 val,
> if (ret)
> return ret;
>
> - /* futex_queue and wait for wakeup, timeout, or a signal. */
> - futex_wait_queue(hb, &q, to);
> + if (flags & FLAGS_SPIN) {
> + ret = futex_spin(hb, &q, to, val);
The empty line below could be removed.
Thanks,
Mathieu
> +
> + if (ret)
> + return ret;
> + } else {
> + /* futex_queue and wait for wakeup, timeout, or a signal. */
> + futex_wait_queue(hb, &q, to);
> + }
>
> /* If we were woken (and unqueued), we succeeded, whatever. */
> if (!futex_unqueue(&q))
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
^ permalink raw reply
* [PATCH v2 1/1] futex: Add FUTEX_SPIN operation
From: André Almeida @ 2024-05-23 20:07 UTC (permalink / raw)
To: Mathieu Desnoyers, Peter Zijlstra
Cc: linux-kernel, Thomas Gleixner, Paul E . McKenney, Boqun Feng,
H . Peter Anvin, Paul Turner, linux-api, Christian Brauner,
Florian Weimer, David.Laight, carlos, Peter Oskolkov,
Alexander Mikhalitsyn, Chris Kennelly, Ingo Molnar, Darren Hart,
Davidlohr Bueso, André Almeida, libc-alpha, Steven Rostedt,
Jonathan Corbet, Noah Goldstein, Daniel Colascione, longman,
kernel-dev
In-Reply-To: <20240523200704.281514-1-andrealmeid@igalia.com>
Add a new mode for futex wait, the futex spin.
Given the FUTEX2_SPIN flag, parse the futex value as the TID of the lock
owner. Then, before going to the normal wait path, spins while the lock
owner is running in a different CPU, to avoid the whole context switch
operation and to quickly return to userspace. If the lock owner is not
running, just sleep as the normal futex wait path.
The user value is masked with FUTEX_TID_MASK, to allow some bits for
future use.
The check for the owner to be running or not is important to avoid
spinning for something that won't be released quickly. Userspace is
responsible on providing the proper TID, the kernel does a basic check.
Signed-off-by: André Almeida <andrealmeid@igalia.com>
---
include/uapi/linux/futex.h | 2 +-
kernel/futex/futex.h | 6 ++-
kernel/futex/waitwake.c | 78 +++++++++++++++++++++++++++++++++++++-
3 files changed, 82 insertions(+), 4 deletions(-)
diff --git a/include/uapi/linux/futex.h b/include/uapi/linux/futex.h
index d2ee625ea189..d77d692ffac2 100644
--- a/include/uapi/linux/futex.h
+++ b/include/uapi/linux/futex.h
@@ -63,7 +63,7 @@
#define FUTEX2_SIZE_U32 0x02
#define FUTEX2_SIZE_U64 0x03
#define FUTEX2_NUMA 0x04
- /* 0x08 */
+#define FUTEX2_SPIN 0x08
/* 0x10 */
/* 0x20 */
/* 0x40 */
diff --git a/kernel/futex/futex.h b/kernel/futex/futex.h
index 8b195d06f4e8..180c1c10dc81 100644
--- a/kernel/futex/futex.h
+++ b/kernel/futex/futex.h
@@ -37,6 +37,7 @@
#define FLAGS_HAS_TIMEOUT 0x0040
#define FLAGS_NUMA 0x0080
#define FLAGS_STRICT 0x0100
+#define FLAGS_SPIN 0x0200
/* FUTEX_ to FLAGS_ */
static inline unsigned int futex_to_flags(unsigned int op)
@@ -52,7 +53,7 @@ static inline unsigned int futex_to_flags(unsigned int op)
return flags;
}
-#define FUTEX2_VALID_MASK (FUTEX2_SIZE_MASK | FUTEX2_PRIVATE)
+#define FUTEX2_VALID_MASK (FUTEX2_SIZE_MASK | FUTEX2_PRIVATE | FUTEX2_SPIN)
/* FUTEX2_ to FLAGS_ */
static inline unsigned int futex2_to_flags(unsigned int flags2)
@@ -65,6 +66,9 @@ static inline unsigned int futex2_to_flags(unsigned int flags2)
if (flags2 & FUTEX2_NUMA)
flags |= FLAGS_NUMA;
+ if (flags2 & FUTEX2_SPIN)
+ flags |= FLAGS_SPIN;
+
return flags;
}
diff --git a/kernel/futex/waitwake.c b/kernel/futex/waitwake.c
index 3a10375d9521..276c03804b92 100644
--- a/kernel/futex/waitwake.c
+++ b/kernel/futex/waitwake.c
@@ -372,6 +372,73 @@ void futex_wait_queue(struct futex_hash_bucket *hb, struct futex_q *q,
__set_current_state(TASK_RUNNING);
}
+static inline bool task_on_cpu(struct task_struct *p)
+{
+#ifdef CONFIG_SMP
+ return !!(p->on_cpu);
+#else
+ return false;
+#endif
+}
+
+static int futex_spin(struct futex_hash_bucket *hb, struct futex_q *q,
+ struct hrtimer_sleeper *timeout, u32 uval)
+{
+ struct task_struct *p;
+ pid_t pid = uval & FUTEX_TID_MASK;
+
+ p = find_get_task_by_vpid(pid);
+
+ /* no task found, maybe it already exited */
+ if (!p) {
+ futex_q_unlock(hb);
+ return -EAGAIN;
+ }
+
+ /* can't spin in a kernel task */
+ if (unlikely(p->flags & PF_KTHREAD)) {
+ put_task_struct(p);
+ futex_q_unlock(hb);
+ return -EPERM;
+ }
+
+ futex_queue(q, hb);
+
+ if (timeout)
+ hrtimer_sleeper_start_expires(timeout, HRTIMER_MODE_ABS);
+
+ while (1) {
+ if (likely(!plist_node_empty(&q->list))) {
+ if (timeout && !timeout->task)
+ goto exit;
+
+ if (task_on_cpu(p)) {
+ /* spin */
+ continue;
+ } else {
+ /* task is not running, sleep */
+ break;
+ }
+ } else {
+ goto exit;
+ }
+ }
+
+ /* spinning didn't work, go to the normal path */
+ set_current_state(TASK_INTERRUPTIBLE|TASK_FREEZABLE);
+
+ if (likely(!plist_node_empty(&q->list))) {
+ if (!timeout || timeout->task)
+ schedule();
+ }
+
+ __set_current_state(TASK_RUNNING);
+
+exit:
+ put_task_struct(p);
+ return 0;
+}
+
/**
* futex_unqueue_multiple - Remove various futexes from their hash bucket
* @v: The list of futexes to unqueue
@@ -665,8 +732,15 @@ int __futex_wait(u32 __user *uaddr, unsigned int flags, u32 val,
if (ret)
return ret;
- /* futex_queue and wait for wakeup, timeout, or a signal. */
- futex_wait_queue(hb, &q, to);
+ if (flags & FLAGS_SPIN) {
+ ret = futex_spin(hb, &q, to, val);
+
+ if (ret)
+ return ret;
+ } else {
+ /* futex_queue and wait for wakeup, timeout, or a signal. */
+ futex_wait_queue(hb, &q, to);
+ }
/* If we were woken (and unqueued), we succeeded, whatever. */
if (!futex_unqueue(&q))
--
2.45.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox