| Message ID | 20240315-dir-deleg-v1-6-a1d6209a3654@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfs, nfsd, nfs: implement directory delegations | expand |
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;
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 <jlayton@kernel.org> > --- > 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 <jlayton@kernel.org> > > --- > > 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.
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);
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 <jlayton@kernel.org> --- fs/namei.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)