| Message ID | 1503697958-6122-3-git-send-email-bfields@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 25 2017, J. Bruce Fields wrote: > From: "J. Bruce Fields" <bfields@redhat.com> > > Pass around a new struct deleg_break_ctl instead of pointers to inode > pointers; in a future patch I want to use this to pass a little more > information from the nfs server to the lease code. The information you are passing from the nfs server to the lease code is largely ignored by the lease code and is passed back to the nfs server, in the sm_breaker_owns_lease call back. If try_break_deleg() passed the 'delegated_inode' pointer though to __break_lease(), it could pass it through any_leases_conflict() and leases_conflict() to the lm_breaker_owns_lease() callback. Then container_of() could be used to access whatever other data nfsd had stashed near the inode. The common code wouldn't need to know any of the details. Just a thought... NeilBrown
On Mon, Aug 28, 2017 at 02:43:05PM +1000, NeilBrown wrote: > On Fri, Aug 25 2017, J. Bruce Fields wrote: > > > From: "J. Bruce Fields" <bfields@redhat.com> > > > > Pass around a new struct deleg_break_ctl instead of pointers to inode > > pointers; in a future patch I want to use this to pass a little more > > information from the nfs server to the lease code. > > The information you are passing from the nfs server to the lease code is > largely ignored by the lease code and is passed back to the nfs server, > in the sm_breaker_owns_lease call back. > > If try_break_deleg() passed the 'delegated_inode' pointer though to > __break_lease(), it could pass it through any_leases_conflict() and > leases_conflict() to the lm_breaker_owns_lease() callback. > Then container_of() could be used to access whatever other data nfsd had > stashed near the inode. The common code wouldn't need to know any of > the details. The new information that we need is some notion of "who" (really, which NFSv4 client) is doing the operation (unlink, whatever) that breaks the lease. We can't get that information from an inode pointer. I may just not understand your suggestion. --b.
On Tue, Aug 29 2017, J. Bruce Fields wrote: > On Mon, Aug 28, 2017 at 02:43:05PM +1000, NeilBrown wrote: >> On Fri, Aug 25 2017, J. Bruce Fields wrote: >> >> > From: "J. Bruce Fields" <bfields@redhat.com> >> > >> > Pass around a new struct deleg_break_ctl instead of pointers to inode >> > pointers; in a future patch I want to use this to pass a little more >> > information from the nfs server to the lease code. >> >> The information you are passing from the nfs server to the lease code is >> largely ignored by the lease code and is passed back to the nfs server, >> in the sm_breaker_owns_lease call back. >> >> If try_break_deleg() passed the 'delegated_inode' pointer though to >> __break_lease(), it could pass it through any_leases_conflict() and >> leases_conflict() to the lm_breaker_owns_lease() callback. >> Then container_of() could be used to access whatever other data nfsd had >> stashed near the inode. The common code wouldn't need to know any of >> the details. > > The new information that we need is some notion of "who" (really, which > NFSv4 client) is doing the operation (unlink, whatever) that breaks the > lease. We can't get that information from an inode pointer. > > I may just not understand your suggestion. Probably I was too terse. I'm suggesting that nfsd have a local "struct deleg_break_ctl" (or whatever name you like) which contains a 'struct inode *delegated_inode' plus whatever else is useful to nfsd. Then nfsd/vfs.c, when it calls things like vfs_unlink(), passes &dbc.delegated_inode (where 'struct deleg_break_ctl dbc'). So the vfs codes doesn't know about 'struct deleg_break_ctl', it just knows about the 'struct inode ** inodep' like it does now, though with the understanding that "DELEG_NO_WAIT" in the **inodep means that same as inodep==NULL. The vfs passes this same 'struct **inode' to lm_breaker_owns_lease() and the nfsd code uses dbc = container_of(inodep, struct deleg_break_ctl, delegated_inode) to get the dbc, and it can use the other fields however it likes. Then instead of the rather task-specific name "lm_breaker_owns_lease" we could have a more general name like "lm_lease_compatible" ... or something. "lm_break_doesn't_see_this_lease_as_being_in_conflict" is a bit long, and contains "'". Thanks, NeilBrown
On Wed, Aug 30, 2017 at 10:43:52AM +1000, NeilBrown wrote: > On Tue, Aug 29 2017, J. Bruce Fields wrote: > > > On Mon, Aug 28, 2017 at 02:43:05PM +1000, NeilBrown wrote: > >> On Fri, Aug 25 2017, J. Bruce Fields wrote: > >> > >> > From: "J. Bruce Fields" <bfields@redhat.com> > >> > > >> > Pass around a new struct deleg_break_ctl instead of pointers to inode > >> > pointers; in a future patch I want to use this to pass a little more > >> > information from the nfs server to the lease code. > >> > >> The information you are passing from the nfs server to the lease code is > >> largely ignored by the lease code and is passed back to the nfs server, > >> in the sm_breaker_owns_lease call back. > >> > >> If try_break_deleg() passed the 'delegated_inode' pointer though to > >> __break_lease(), it could pass it through any_leases_conflict() and > >> leases_conflict() to the lm_breaker_owns_lease() callback. > >> Then container_of() could be used to access whatever other data nfsd had > >> stashed near the inode. The common code wouldn't need to know any of > >> the details. > > > > The new information that we need is some notion of "who" (really, which > > NFSv4 client) is doing the operation (unlink, whatever) that breaks the > > lease. We can't get that information from an inode pointer. > > > > I may just not understand your suggestion. > > Probably I was too terse. > > I'm suggesting that nfsd have a local "struct deleg_break_ctl" (or > whatever name you like) which contains a 'struct inode *delegated_inode' > plus whatever else is useful to nfsd. > Then nfsd/vfs.c, when it calls things like vfs_unlink(), passes > &dbc.delegated_inode > (where 'struct deleg_break_ctl dbc'). > So the vfs codes doesn't know about 'struct deleg_break_ctl', it just > knows about the 'struct inode ** inodep' like it does now, though with the > understanding that "DELEG_NO_WAIT" in the **inodep means that same as > inodep==NULL. > > The vfs passes this same 'struct **inode' to lm_breaker_owns_lease() and > the nfsd code uses > dbc = container_of(inodep, struct deleg_break_ctl, delegated_inode) > to get the dbc, and it can use the other fields however it likes. Oh, now I understand. That's an interesting idea. I don't *think* it works on its own, because I don't think we've got a way in that case to know whether the passed-down delegated inode came from nfsd (and thus is contained in a deleg_break_ctl structure). We get the lm_breaker_owns_lease operation from the lease that's already set on the inode, but we don't know who that breaking operation is coming from. Maybe something alon those lines could be made to work. > Then instead of the rather task-specific name "lm_breaker_owns_lease" we > could have a more general name like "lm_lease_compatible" ... or > something. "lm_break_doesn't_see_this_lease_as_being_in_conflict" is a > bit long, and contains "'". Hah, yes.--b.
On Wed, Aug 30 2017, J. Bruce Fields wrote: > On Wed, Aug 30, 2017 at 10:43:52AM +1000, NeilBrown wrote: >> On Tue, Aug 29 2017, J. Bruce Fields wrote: >> >> > On Mon, Aug 28, 2017 at 02:43:05PM +1000, NeilBrown wrote: >> >> On Fri, Aug 25 2017, J. Bruce Fields wrote: >> >> >> >> > From: "J. Bruce Fields" <bfields@redhat.com> >> >> > >> >> > Pass around a new struct deleg_break_ctl instead of pointers to inode >> >> > pointers; in a future patch I want to use this to pass a little more >> >> > information from the nfs server to the lease code. >> >> >> >> The information you are passing from the nfs server to the lease code is >> >> largely ignored by the lease code and is passed back to the nfs server, >> >> in the sm_breaker_owns_lease call back. >> >> >> >> If try_break_deleg() passed the 'delegated_inode' pointer though to >> >> __break_lease(), it could pass it through any_leases_conflict() and >> >> leases_conflict() to the lm_breaker_owns_lease() callback. >> >> Then container_of() could be used to access whatever other data nfsd had >> >> stashed near the inode. The common code wouldn't need to know any of >> >> the details. >> > >> > The new information that we need is some notion of "who" (really, which >> > NFSv4 client) is doing the operation (unlink, whatever) that breaks the >> > lease. We can't get that information from an inode pointer. >> > >> > I may just not understand your suggestion. >> >> Probably I was too terse. >> >> I'm suggesting that nfsd have a local "struct deleg_break_ctl" (or >> whatever name you like) which contains a 'struct inode *delegated_inode' >> plus whatever else is useful to nfsd. >> Then nfsd/vfs.c, when it calls things like vfs_unlink(), passes >> &dbc.delegated_inode >> (where 'struct deleg_break_ctl dbc'). >> So the vfs codes doesn't know about 'struct deleg_break_ctl', it just >> knows about the 'struct inode ** inodep' like it does now, though with the >> understanding that "DELEG_NO_WAIT" in the **inodep means that same as >> inodep==NULL. >> >> The vfs passes this same 'struct **inode' to lm_breaker_owns_lease() and >> the nfsd code uses >> dbc = container_of(inodep, struct deleg_break_ctl, delegated_inode) >> to get the dbc, and it can use the other fields however it likes. > > Oh, now I understand. That's an interesting idea. I don't *think* it > works on its own, because I don't think we've got a way in that case to > know whether the passed-down delegated inode came from nfsd (and thus is > contained in a deleg_break_ctl structure). We get the > lm_breaker_owns_lease operation from the lease that's already set on the > inode, but we don't know who that breaking operation is coming from. That is a perfectly valid criticism and one that, I think, applies equally to your original code. +static bool nfsd_breaker_owns_lease(void *who, struct file_lock *fl) +{ + struct svc_rqst *rqst = who; How does nfsd know that 'who' is an svc_rqst?? You can save your code by passing nfsd4_client_from_rqst(rqst) as the 'who', and then testing + if (dl->dl_stid.sc_client != who) { in the loop in nfsd_breaker_owns_lease. So the only action performed on the void* is an equality test. I cannot save my code quite so easily. :-( Thanks, NeilBrown > > Maybe something alon those lines could be made to work. > >> Then instead of the rather task-specific name "lm_breaker_owns_lease" we >> could have a more general name like "lm_lease_compatible" ... or >> something. "lm_break_doesn't_see_this_lease_as_being_in_conflict" is a >> bit long, and contains "'". > > Hah, yes.--b. > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 31, 2017 at 09:26:28AM +1000, NeilBrown wrote: > On Wed, Aug 30 2017, J. Bruce Fields wrote: > > > On Wed, Aug 30, 2017 at 10:43:52AM +1000, NeilBrown wrote: > >> I'm suggesting that nfsd have a local "struct deleg_break_ctl" (or > >> whatever name you like) which contains a 'struct inode *delegated_inode' > >> plus whatever else is useful to nfsd. > >> Then nfsd/vfs.c, when it calls things like vfs_unlink(), passes > >> &dbc.delegated_inode > >> (where 'struct deleg_break_ctl dbc'). > >> So the vfs codes doesn't know about 'struct deleg_break_ctl', it just > >> knows about the 'struct inode ** inodep' like it does now, though with the > >> understanding that "DELEG_NO_WAIT" in the **inodep means that same as > >> inodep==NULL. > >> > >> The vfs passes this same 'struct **inode' to lm_breaker_owns_lease() and > >> the nfsd code uses > >> dbc = container_of(inodep, struct deleg_break_ctl, delegated_inode) > >> to get the dbc, and it can use the other fields however it likes. > > > > Oh, now I understand. That's an interesting idea. I don't *think* it > > works on its own, because I don't think we've got a way in that case to > > know whether the passed-down delegated inode came from nfsd (and thus is > > contained in a deleg_break_ctl structure). We get the > > lm_breaker_owns_lease operation from the lease that's already set on the > > inode, but we don't know who that breaking operation is coming from. > > That is a perfectly valid criticism and one that, I think, applies > equally to your original code. > > +static bool nfsd_breaker_owns_lease(void *who, struct file_lock *fl) > +{ > + struct svc_rqst *rqst = who; > > How does nfsd know that 'who' is an svc_rqst?? Only nfsd fills in the "who" field of deleg_break_ctl. But non-nfsd users do need to pass a non-NULL delegated_inode. > You can save your code by passing > nfsd4_client_from_rqst(rqst) > > as the 'who', and then testing > + if (dl->dl_stid.sc_client != who) { > > in the loop in nfsd_breaker_owns_lease. So the only action performed > on the void* is an equality test. Yes, that sounds more robust, thanks for the suggestion. --b.
On Thu, Aug 31 2017, J. Bruce Fields wrote: > On Thu, Aug 31, 2017 at 09:26:28AM +1000, NeilBrown wrote: >> On Wed, Aug 30 2017, J. Bruce Fields wrote: >> >> > On Wed, Aug 30, 2017 at 10:43:52AM +1000, NeilBrown wrote: >> >> I'm suggesting that nfsd have a local "struct deleg_break_ctl" (or >> >> whatever name you like) which contains a 'struct inode *delegated_inode' >> >> plus whatever else is useful to nfsd. >> >> Then nfsd/vfs.c, when it calls things like vfs_unlink(), passes >> >> &dbc.delegated_inode >> >> (where 'struct deleg_break_ctl dbc'). >> >> So the vfs codes doesn't know about 'struct deleg_break_ctl', it just >> >> knows about the 'struct inode ** inodep' like it does now, though with the >> >> understanding that "DELEG_NO_WAIT" in the **inodep means that same as >> >> inodep==NULL. >> >> >> >> The vfs passes this same 'struct **inode' to lm_breaker_owns_lease() and >> >> the nfsd code uses >> >> dbc = container_of(inodep, struct deleg_break_ctl, delegated_inode) >> >> to get the dbc, and it can use the other fields however it likes. >> > >> > Oh, now I understand. That's an interesting idea. I don't *think* it >> > works on its own, because I don't think we've got a way in that case to >> > know whether the passed-down delegated inode came from nfsd (and thus is >> > contained in a deleg_break_ctl structure). We get the >> > lm_breaker_owns_lease operation from the lease that's already set on the >> > inode, but we don't know who that breaking operation is coming from. >> >> That is a perfectly valid criticism and one that, I think, applies >> equally to your original code. >> >> +static bool nfsd_breaker_owns_lease(void *who, struct file_lock *fl) >> +{ >> + struct svc_rqst *rqst = who; >> >> How does nfsd know that 'who' is an svc_rqst?? > > Only nfsd fills in the "who" field of deleg_break_ctl. But non-nfsd > users do need to pass a non-NULL delegated_inode. Yes, of course... So having been wrong about this code twice, I'm starting to get a feel for what it does and why. I still wonder if there might be a better approach though. You are changing the interface to pass a magic cookie with the meaning "Don't bother breaking a delegation which matches this magic cookie". Would it not be better to pass a delegation, and say "Don't bother breaking this delegation". And if it were a WRITE delegation, that could be optimised as "don't bother breaking any delegation, I have a write delegation so I have exclusive access". Whenever we call a vfs_* function that will need to break delegations we have already done the lookup and have the dentry and inode, so finding a delegation shouldn't be prohibitive. nfsd would need to find that delegation, prevent further delegations being handed out, and check that there aren't already conflicting delegations. If there are conflicts, recall them. Once there are no conflicting delegations, make the vfs_ request. One downside of this is that nfsd delegations would be recalled before any others, rather than doing them all in parallel. This could be addressed by calling try_break_deleg() when recalling the nfsd delegations. This approach seems to be half-way between your original attempt that you described, which is racy, and the attempt you posted which adds the callback that I don't particularly like. ??? Thanks, NeilBrown
diff --git a/fs/attr.c b/fs/attr.c index 135304146120..255315dbca32 100644 --- a/fs/attr.c +++ b/fs/attr.c @@ -185,23 +185,23 @@ EXPORT_SYMBOL(setattr_copy); * notify_change - modify attributes of a filesytem object * @dentry: object affected * @iattr: new attributes - * @delegated_inode: returns inode, if the inode is delegated + * @deleg_break_ctl: used to return inode, if the inode is delegated * * The caller must hold the i_mutex on the affected object. * * If notify_change discovers a delegation in need of breaking, * it will return -EWOULDBLOCK and return a reference to the inode in - * delegated_inode. The caller should then break the delegation and + * deleg_break_ctl. The caller should then break the delegation and * retry. Because breaking a delegation may take a long time, the * caller should drop the i_mutex before doing so. * - * Alternatively, a caller may pass NULL for delegated_inode. This may + * Alternatively, a caller may pass NULL for deleg_break_ctl. This may * be appropriate for callers that expect the underlying filesystem not * to be NFS exported. Also, passing NULL is fine for callers holding * the file open for write, as there can be no conflicting delegation in * that case. */ -int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **delegated_inode) +int notify_change(struct dentry * dentry, struct iattr * attr, struct deleg_break_ctl *deleg_break_ctl) { struct inode *inode = dentry->d_inode; umode_t mode = inode->i_mode; @@ -304,7 +304,7 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de error = security_inode_setattr(dentry, attr); if (error) return error; - error = try_break_deleg(inode, delegated_inode); + error = try_break_deleg(inode, deleg_break_ctl); if (error) return error; diff --git a/fs/namei.c b/fs/namei.c index 5a93be7b2c9c..a75ab583aee7 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -3941,21 +3941,21 @@ SYSCALL_DEFINE1(rmdir, const char __user *, pathname) * vfs_unlink - unlink a filesystem object * @dir: parent directory * @dentry: victim - * @delegated_inode: returns victim inode, if the inode is delegated. + * @deleg_break_ctl: used to return victim inode, if the inode is delegated. * * The caller must hold dir->i_mutex. * * If vfs_unlink discovers a delegation, it will return -EWOULDBLOCK and - * return a reference to the inode in delegated_inode. The caller + * return a reference to the inode in deleg_break_ctl. The caller * should then break the delegation on that inode and retry. Because * breaking a delegation may take a long time, the caller should drop * dir->i_mutex before doing so. * - * Alternatively, a caller may pass NULL for delegated_inode. This may + * Alternatively, a caller may pass NULL for deleg_break_ctl. This may * be appropriate for callers that expect the underlying filesystem not * to be NFS exported. */ -int vfs_unlink(struct inode *dir, struct dentry *dentry, struct inode **delegated_inode) +int vfs_unlink(struct inode *dir, struct dentry *dentry, struct deleg_break_ctl *deleg_break_ctl) { struct inode *target = dentry->d_inode; int error = may_delete(dir, dentry, 0); @@ -3972,7 +3972,7 @@ int vfs_unlink(struct inode *dir, struct dentry *dentry, struct inode **delegate else { error = security_inode_unlink(dir, dentry); if (!error) { - error = try_break_deleg(target, delegated_inode); + error = try_break_deleg(target, deleg_break_ctl); if (error) goto out; error = dir->i_op->unlink(dir, dentry); @@ -4010,7 +4010,7 @@ static long do_unlinkat(int dfd, const char __user *pathname) struct qstr last; int type; struct inode *inode = NULL; - struct inode *delegated_inode = NULL; + struct deleg_break_ctl deleg_break_ctl = {}; unsigned int lookup_flags = 0; retry: name = filename_parentat(dfd, getname(pathname), lookup_flags, @@ -4040,7 +4040,7 @@ static long do_unlinkat(int dfd, const char __user *pathname) error = security_path_unlink(&path, dentry); if (error) goto exit2; - error = vfs_unlink(path.dentry->d_inode, dentry, &delegated_inode); + error = vfs_unlink(path.dentry->d_inode, dentry, &deleg_break_ctl); exit2: dput(dentry); } @@ -4048,7 +4048,7 @@ static long do_unlinkat(int dfd, const char __user *pathname) if (inode) iput(inode); /* truncate the inode here */ inode = NULL; - error = break_deleg_wait(&delegated_inode, error); + error = break_deleg_wait(&deleg_break_ctl, error); if (error == DELEG_RETRY) goto retry_deleg; mnt_drop_write(path.mnt); @@ -4150,21 +4150,21 @@ SYSCALL_DEFINE2(symlink, const char __user *, oldname, const char __user *, newn * @old_dentry: object to be linked * @dir: new parent * @new_dentry: where to create the new link - * @delegated_inode: returns inode needing a delegation break + * @deleg_break_ctl: returns inode needing a delegation break * * The caller must hold dir->i_mutex * * If vfs_link discovers a delegation on the to-be-linked file in need * of breaking, it will return -EWOULDBLOCK and return a reference to the - * inode in delegated_inode. The caller should then break the delegation + * inode in deleg_break_ctl. The caller should then break the delegation * and retry. Because breaking a delegation may take a long time, the * caller should drop the i_mutex before doing so. * - * Alternatively, a caller may pass NULL for delegated_inode. This may + * Alternatively, a caller may pass NULL for deleg_break_ctl. This may * be appropriate for callers that expect the underlying filesystem not * to be NFS exported. */ -int vfs_link(struct dentry *old_dentry, struct inode *dir, struct dentry *new_dentry, struct inode **delegated_inode) +int vfs_link(struct dentry *old_dentry, struct inode *dir, struct dentry *new_dentry, struct deleg_break_ctl *deleg_break_ctl) { struct inode *inode = old_dentry->d_inode; unsigned max_links = dir->i_sb->s_max_links; @@ -4208,7 +4208,7 @@ int vfs_link(struct dentry *old_dentry, struct inode *dir, struct dentry *new_de else if (max_links && inode->i_nlink >= max_links) error = -EMLINK; else { - error = try_break_deleg(inode, delegated_inode); + error = try_break_deleg(inode, deleg_break_ctl); if (!error) error = dir->i_op->link(old_dentry, dir, new_dentry); } @@ -4239,7 +4239,7 @@ SYSCALL_DEFINE5(linkat, int, olddfd, const char __user *, oldname, { struct dentry *new_dentry; struct path old_path, new_path; - struct inode *delegated_inode = NULL; + struct deleg_break_ctl deleg_break_ctl = {}; int how = 0; int error; @@ -4278,10 +4278,10 @@ SYSCALL_DEFINE5(linkat, int, olddfd, const char __user *, oldname, error = security_path_link(old_path.dentry, &new_path, new_dentry); if (error) goto out_dput; - error = vfs_link(old_path.dentry, new_path.dentry->d_inode, new_dentry, &delegated_inode); + error = vfs_link(old_path.dentry, new_path.dentry->d_inode, new_dentry, &deleg_break_ctl); out_dput: done_path_create(&new_path, new_dentry); - error = break_deleg_wait(&delegated_inode, error); + error = break_deleg_wait(&deleg_break_ctl, error); if (error == DELEG_RETRY) { path_put(&old_path); goto retry; @@ -4308,19 +4308,19 @@ SYSCALL_DEFINE2(link, const char __user *, oldname, const char __user *, newname * @old_dentry: source * @new_dir: parent of destination * @new_dentry: destination - * @delegated_inode: returns an inode needing a delegation break + * @deleg_break_ctl: used to return an inode needing a delegation break * @flags: rename flags * * The caller must hold multiple mutexes--see lock_rename()). * * If vfs_rename discovers a delegation in need of breaking at either * the source or destination, it will return -EWOULDBLOCK and return a - * reference to the inode in delegated_inode. The caller should then + * reference to the inode in deleg_break_ctl. The caller should then * break the delegation and retry. Because breaking a delegation may * take a long time, the caller should drop all locks before doing * so. * - * Alternatively, a caller may pass NULL for delegated_inode. This may + * Alternatively, a caller may pass NULL for deleg_break_ctl. This may * be appropriate for callers that expect the underlying filesystem not * to be NFS exported. * @@ -4354,7 +4354,7 @@ SYSCALL_DEFINE2(link, const char __user *, oldname, const char __user *, newname */ int vfs_rename(struct inode *old_dir, struct dentry *old_dentry, struct inode *new_dir, struct dentry *new_dentry, - struct inode **delegated_inode, unsigned int flags) + struct deleg_break_ctl *deleg_break_ctl, unsigned int flags) { int error; bool is_dir = d_is_dir(old_dentry); @@ -4431,12 +4431,12 @@ int vfs_rename(struct inode *old_dir, struct dentry *old_dentry, if (is_dir && !(flags & RENAME_EXCHANGE) && target) shrink_dcache_parent(new_dentry); if (!is_dir) { - error = try_break_deleg(source, delegated_inode); + error = try_break_deleg(source, deleg_break_ctl); if (error) goto out; } if (target && !new_is_dir) { - error = try_break_deleg(target, delegated_inode); + error = try_break_deleg(target, deleg_break_ctl); if (error) goto out; } @@ -4485,7 +4485,7 @@ SYSCALL_DEFINE5(renameat2, int, olddfd, const char __user *, oldname, struct path old_path, new_path; struct qstr old_last, new_last; int old_type, new_type; - struct inode *delegated_inode = NULL; + struct deleg_break_ctl deleg_break_ctl = {}; struct filename *from; struct filename *to; unsigned int lookup_flags = 0, target_flags = LOOKUP_RENAME_TARGET; @@ -4590,14 +4590,14 @@ SYSCALL_DEFINE5(renameat2, int, olddfd, const char __user *, oldname, goto exit5; error = vfs_rename(old_path.dentry->d_inode, old_dentry, new_path.dentry->d_inode, new_dentry, - &delegated_inode, flags); + &deleg_break_ctl, flags); exit5: dput(new_dentry); exit4: dput(old_dentry); exit3: unlock_rename(new_path.dentry, old_path.dentry); - error = break_deleg_wait(&delegated_inode, error); + error = break_deleg_wait(&deleg_break_ctl, error); if (error == DELEG_RETRY) goto retry_deleg; mnt_drop_write(old_path.mnt); diff --git a/fs/open.c b/fs/open.c index d49e9385e45d..6c6443476316 100644 --- a/fs/open.c +++ b/fs/open.c @@ -515,7 +515,7 @@ SYSCALL_DEFINE1(chroot, const char __user *, filename) static int chmod_common(const struct path *path, umode_t mode) { struct inode *inode = path->dentry->d_inode; - struct inode *delegated_inode = NULL; + struct deleg_break_ctl deleg_break_ctl = {}; struct iattr newattrs; int error; @@ -529,10 +529,10 @@ static int chmod_common(const struct path *path, umode_t mode) goto out_unlock; newattrs.ia_mode = (mode & S_IALLUGO) | (inode->i_mode & ~S_IALLUGO); newattrs.ia_valid = ATTR_MODE | ATTR_CTIME; - error = notify_change(path->dentry, &newattrs, &delegated_inode); + error = notify_change(path->dentry, &newattrs, &deleg_break_ctl); out_unlock: inode_unlock(inode); - error = break_deleg_wait(&delegated_inode, error); + error = break_deleg_wait(&deleg_break_ctl, error); if (error == DELEG_RETRY) goto retry_deleg; mnt_drop_write(path->mnt); @@ -578,7 +578,7 @@ SYSCALL_DEFINE2(chmod, const char __user *, filename, umode_t, mode) static int chown_common(const struct path *path, uid_t user, gid_t group) { struct inode *inode = path->dentry->d_inode; - struct inode *delegated_inode = NULL; + struct deleg_break_ctl deleg_break_ctl = {}; int error; struct iattr newattrs; kuid_t uid; @@ -607,9 +607,9 @@ static int chown_common(const struct path *path, uid_t user, gid_t group) inode_lock(inode); error = security_path_chown(path, uid, gid); if (!error) - error = notify_change(path->dentry, &newattrs, &delegated_inode); + error = notify_change(path->dentry, &newattrs, &deleg_break_ctl); inode_unlock(inode); - error = break_deleg_wait(&delegated_inode, error); + error = break_deleg_wait(&deleg_break_ctl, error); if (error == DELEG_RETRY) goto retry_deleg; return error; diff --git a/fs/utimes.c b/fs/utimes.c index 75467b7ebfce..9af7ca3810db 100644 --- a/fs/utimes.c +++ b/fs/utimes.c @@ -49,7 +49,7 @@ static int utimes_common(const struct path *path, struct timespec *times) int error; struct iattr newattrs; struct inode *inode = path->dentry->d_inode; - struct inode *delegated_inode = NULL; + struct deleg_break_ctl deleg_break_ctl = {}; error = mnt_want_write(path->mnt); if (error) @@ -87,9 +87,9 @@ static int utimes_common(const struct path *path, struct timespec *times) } retry_deleg: inode_lock(inode); - error = notify_change(path->dentry, &newattrs, &delegated_inode); + error = notify_change(path->dentry, &newattrs, &deleg_break_ctl); inode_unlock(inode); - error = break_deleg_wait(&delegated_inode, error); + error = break_deleg_wait(&deleg_break_ctl, error); if (error == DELEG_RETRY) goto retry_deleg; diff --git a/include/linux/fs.h b/include/linux/fs.h index 1d0d2fde1766..20a07375e60c 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1564,6 +1564,11 @@ static inline void sb_start_intwrite(struct super_block *sb) extern bool inode_owner_or_capable(const struct inode *inode); +/* Used to pass some information used by NFSv4 delegations */ +struct deleg_break_ctl { + struct inode *delegated_inode; /* inode with in-progress break */ +}; + /* * VFS helper functions.. */ @@ -1571,10 +1576,10 @@ extern int vfs_create(struct inode *, struct dentry *, umode_t, bool); extern int vfs_mkdir(struct inode *, struct dentry *, umode_t); extern int vfs_mknod(struct inode *, struct dentry *, umode_t, dev_t); extern int vfs_symlink(struct inode *, struct dentry *, const char *); -extern int vfs_link(struct dentry *, struct inode *, struct dentry *, struct inode **); +extern int vfs_link(struct dentry *, struct inode *, struct dentry *, struct deleg_break_ctl *); extern int vfs_rmdir(struct inode *, struct dentry *); -extern int vfs_unlink(struct inode *, struct dentry *, struct inode **); -extern int vfs_rename(struct inode *, struct dentry *, struct inode *, struct dentry *, struct inode **, unsigned int); +extern int vfs_unlink(struct inode *, struct dentry *, struct deleg_break_ctl *); +extern int vfs_rename(struct inode *, struct dentry *, struct inode *, struct dentry *, struct deleg_break_ctl *, unsigned int); extern int vfs_whiteout(struct inode *, struct dentry *); extern struct dentry *vfs_tmpfile(struct dentry *dentry, umode_t mode, @@ -2276,13 +2281,13 @@ static inline int break_deleg(struct inode *inode, unsigned int mode) return 0; } -static inline int try_break_deleg(struct inode *inode, struct inode **delegated_inode) +static inline int try_break_deleg(struct inode *inode, struct deleg_break_ctl *deleg_break_ctl) { int ret; ret = break_deleg(inode, O_WRONLY|O_NONBLOCK); - if (ret == -EWOULDBLOCK && delegated_inode) { - *delegated_inode = inode; + if (ret == -EWOULDBLOCK && deleg_break_ctl) { + deleg_break_ctl->delegated_inode = inode; ihold(inode); } return ret; @@ -2290,13 +2295,14 @@ static inline int try_break_deleg(struct inode *inode, struct inode **delegated_ #define DELEG_RETRY 1 -static inline int break_deleg_wait(struct inode **delegated_inode, int error) +static inline int break_deleg_wait(struct deleg_break_ctl *deleg_break_ctl, int error) { - if (!*delegated_inode) + if (!deleg_break_ctl->delegated_inode) return error; - error = break_deleg(*delegated_inode, O_WRONLY); - iput(*delegated_inode); - *delegated_inode = NULL; + + error = break_deleg(deleg_break_ctl->delegated_inode, O_WRONLY); + iput(deleg_break_ctl->delegated_inode); + deleg_break_ctl->delegated_inode = NULL; return error ? error : DELEG_RETRY; } @@ -2321,12 +2327,12 @@ static inline int break_deleg(struct inode *inode, unsigned int mode) return 0; } -static inline int try_break_deleg(struct inode *inode, struct inode **delegated_inode) +static inline int try_break_deleg(struct inode *inode, struct deleg_break_ctl *deleg_break_ctl) { return 0; } -static inline int break_deleg_wait(struct inode **delegated_inode) +static inline int break_deleg_wait(struct deleg_break_ctl *deleg_break_ctl, int error) { BUG(); return 0; @@ -2639,7 +2645,7 @@ extern void emergency_remount(void); #ifdef CONFIG_BLOCK extern sector_t bmap(struct inode *, sector_t); #endif -extern int notify_change(struct dentry *, struct iattr *, struct inode **); +extern int notify_change(struct dentry *, struct iattr *, struct deleg_break_ctl *); extern int inode_permission(struct inode *, int); extern int __inode_permission(struct inode *, int); extern int generic_permission(struct inode *, int);