In order to add directory delegation support, we need to break
delegations on the parent whenever there is going to be a change in the
directory.
Add a delegated_inode parameter to lookup_open and have it break the
delegation. Then, open_last_lookups can wait for the delegation break
and retry the call to lookup_open once it's done.
Signed-off-by: Jeff Layton <[email protected]>
---
fs/namei.c | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index f00d8d708001..88598a62ec64 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3404,7 +3404,7 @@ static struct dentry *atomic_open(struct nameidata *nd, struct dentry *dentry,
*/
static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
const struct open_flags *op,
- bool got_write)
+ bool got_write, struct inode **delegated_inode)
{
struct mnt_idmap *idmap;
struct dentry *dir = nd->path.dentry;
@@ -3490,6 +3490,11 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
/* Negative dentry, just create the file */
if (!dentry->d_inode && (open_flag & O_CREAT)) {
+ /* but break the directory lease first! */
+ error = try_break_deleg(dir_inode, delegated_inode);
+ if (error)
+ goto out_dput;
+
file->f_mode |= FMODE_CREATED;
audit_inode_child(dir_inode, dentry, AUDIT_TYPE_CHILD_CREATE);
if (!dir_inode->i_op->create) {
@@ -3517,6 +3522,7 @@ static const char *open_last_lookups(struct nameidata *nd,
struct file *file, const struct open_flags *op)
{
struct dentry *dir = nd->path.dentry;
+ struct inode *delegated_inode = NULL;
int open_flag = op->open_flag;
bool got_write = false;
struct dentry *dentry;
@@ -3553,7 +3559,7 @@ static const char *open_last_lookups(struct nameidata *nd,
if (unlikely(nd->last.name[nd->last.len]))
return ERR_PTR(-EISDIR);
}
-
+retry:
if (open_flag & (O_CREAT | O_TRUNC | O_WRONLY | O_RDWR)) {
got_write = !mnt_want_write(nd->path.mnt);
/*
@@ -3566,7 +3572,7 @@ static const char *open_last_lookups(struct nameidata *nd,
inode_lock(dir->d_inode);
else
inode_lock_shared(dir->d_inode);
- dentry = lookup_open(nd, file, op, got_write);
+ dentry = lookup_open(nd, file, op, got_write, &delegated_inode);
if (!IS_ERR(dentry) && (file->f_mode & FMODE_CREATED))
fsnotify_create(dir->d_inode, dentry);
if (open_flag & O_CREAT)
@@ -3577,8 +3583,16 @@ static const char *open_last_lookups(struct nameidata *nd,
if (got_write)
mnt_drop_write(nd->path.mnt);
- if (IS_ERR(dentry))
+ if (IS_ERR(dentry)) {
+ if (delegated_inode) {
+ int error = break_deleg_wait(&delegated_inode);
+
+ if (!error)
+ goto retry;
+ return ERR_PTR(error);
+ }
return ERR_CAST(dentry);
+ }
if (file->f_mode & (FMODE_OPENED | FMODE_CREATED)) {
dput(nd->path.dentry);
--
2.44.0
On Fri, Mar 15, 2024 at 12:52:57PM -0400, Jeff Layton wrote:
> In order to add directory delegation support, we need to break
> delegations on the parent whenever there is going to be a change in the
> directory.
>
> Add a delegated_inode parameter to lookup_open and have it break the
> delegation. Then, open_last_lookups can wait for the delegation break
> and retry the call to lookup_open once it's done.
> @@ -3490,6 +3490,11 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
Wait a sec - are you going to do anything to the atomic_open side of things?
> /* Negative dentry, just create the file */
> if (!dentry->d_inode && (open_flag & O_CREAT)) {
> + /* but break the directory lease first! */
> + error = try_break_deleg(dir_inode, delegated_inode);
> + if (error)
> + goto out_dput;
On Sun, 2024-03-17 at 00:19 +0000, Al Viro wrote:
> On Fri, Mar 15, 2024 at 12:52:57PM -0400, Jeff Layton wrote:
> > In order to add directory delegation support, we need to break
> > delegations on the parent whenever there is going to be a change in the
> > directory.
> >
> > Add a delegated_inode parameter to lookup_open and have it break the
> > delegation. Then, open_last_lookups can wait for the delegation break
> > and retry the call to lookup_open once it's done.
>
> > @@ -3490,6 +3490,11 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
>
> Wait a sec - are you going to do anything to the atomic_open side of things?
>
>
Hmm good point. I was thinking that all of the filesystems that had
atomic_open didn't support leases. I'm wrong though -- there are some
that currently do:
9p: It's a network filesystem, and I don't think it has any sort of
asynchronous notification or delegation-like object, does it? It might
be best though to just make it call simple_nosetlease.
fuse: fuse allows leases today. I doubt we can get away with turning
that off now. There probably ought to be a way for the userland driver
to opt-in or out of allowing built-in lease support maybe a flag or
something?
ntfs3: IDGI. Why does ntfs3 (which is a local filesystem, unless I'm
mistaken) have an atomic_open? Shouldn't lookup+open be fine, like with
most local filesystems?
vboxsf: Probably the same situation as 9p. Can we just disable leases?
I'll spin up a patchset soon to add proper setlease handlers to all of
the above. Then we can then guard against allowing generic_setlease on
filesystems by default on filesystems with an atomic_open handler.
Another (maybe better) idea might be to require filesystems to specify a
setlease handler if they want them enabled. We could just set the
existing local filesystems to generic_setlease. That would make lease
support a strictly opt-in thing, which is probably the best idea for
avoiding surprises with them.
>
> > /* Negative dentry, just create the file */
> > if (!dentry->d_inode && (open_flag & O_CREAT)) {
> > + /* but break the directory lease first! */
> > + error = try_break_deleg(dir_inode, delegated_inode);
> > + if (error)
> > + goto out_dput;
--
Jeff Layton <[email protected]>
Hi Jeff,
> In order to add directory delegation support, we need to break
> delegations on the parent whenever there is going to be a change in the
> directory.
>
> Add a delegated_inode parameter to lookup_open and have it break the
> delegation. Then, open_last_lookups can wait for the delegation break
> and retry the call to lookup_open once it's done.
>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> fs/namei.c | 22 ++++++++++++++++++----
> 1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/fs/namei.c b/fs/namei.c
> index f00d8d708001..88598a62ec64 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3404,7 +3404,7 @@ static struct dentry *atomic_open(struct nameidata *nd, struct dentry *dentry,
> */
> static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
> const struct open_flags *op,
> - bool got_write)
> + bool got_write, struct inode **delegated_inode)
Does NFS has a concept of lease keys and parent lease keys?
In SMB it's possible that the client passes a lease key (16 client chosen bytes) to a directory open,
when asking for a directory lease.
Then operations on files within that directory, take that lease key from the directory as
'parent lease keys' in addition to a unique lease key for the file.
That way a client can avoid breaking its own directory leases when creating/move/delete... files
in the directory.
metze
On Mon, 2024-03-18 at 09:25 +0100, Stefan Metzmacher wrote:
> Hi Jeff,
>
> > In order to add directory delegation support, we need to break
> > delegations on the parent whenever there is going to be a change in the
> > directory.
> >
> > Add a delegated_inode parameter to lookup_open and have it break the
> > delegation. Then, open_last_lookups can wait for the delegation break
> > and retry the call to lookup_open once it's done.
> >
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > fs/namei.c | 22 ++++++++++++++++++----
> > 1 file changed, 18 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/namei.c b/fs/namei.c
> > index f00d8d708001..88598a62ec64 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -3404,7 +3404,7 @@ static struct dentry *atomic_open(struct nameidata *nd, struct dentry *dentry,
> > */
> > static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
> > const struct open_flags *op,
> > - bool got_write)
> > + bool got_write, struct inode **delegated_inode)
>
> Does NFS has a concept of lease keys and parent lease keys?
>
> In SMB it's possible that the client passes a lease key (16 client chosen bytes) to a directory open,
> when asking for a directory lease.
>
> Then operations on files within that directory, take that lease key from the directory as
> 'parent lease keys' in addition to a unique lease key for the file.
>
> That way a client can avoid breaking its own directory leases when creating/move/delete... files
> in the directory.
>
No, it's a bit different with NFSv4 directory delegations. A delegation
is given vs a filehandle (which is analogous to an inode), and it gets a
stateid, which just uniquely identifies it. There is no real association
with the parent.
When you request the dir delegation, you can request to be notified when
something changes instead of the server recalling it. The server may or
may not grant that request. Notifications are not implemented in this
patchset as of yet. I'm focusing on getting the recall handling right
first, and then I'll plan to add that in a later phase.
--
Jeff Layton <[email protected]>