* [PATCH] nfsd: Prevent truncation of an unlinked inode from blocking access to its directory
@ 2021-05-14 3:58 Nick Huang
2021-05-14 15:09 ` J. Bruce Fields
2021-05-14 15:46 ` Trond Myklebust
0 siblings, 2 replies; 11+ messages in thread
From: Nick Huang @ 2021-05-14 3:58 UTC (permalink / raw)
To: bfields, chuck.lever
Cc: linux-nfs, Yu Hsiang Huang, Bing Jing Chang, Robbie Ko
From: Yu Hsiang Huang <nickhuang@synology.com>
Truncation of an unlinked inode may take a long time for I/O waiting, and
it doesn't have to prevent access to the directory. Thus, let truncation
occur outside the directory's mutex, just like do_unlinkat() does.
Signed-off-by: Yu Hsiang Huang <nickhuang@synology.com>
Signed-off-by: Bing Jing Chang <bingjingc@synology.com>
Signed-off-by: Robbie Ko <robbieko@synology.com>
---
fs/nfsd/vfs.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 15adf1f6ab21..39948f130712 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1859,6 +1859,7 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
{
struct dentry *dentry, *rdentry;
struct inode *dirp;
+ struct inode *rinode;
__be32 err;
int host_err;
@@ -1887,6 +1888,8 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
host_err = -ENOENT;
goto out_drop_write;
}
+ rinode = d_inode(rdentry);
+ ihold(rinode);
if (!type)
type = d_inode(rdentry)->i_mode & S_IFMT;
@@ -1902,6 +1905,8 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
if (!host_err)
host_err = commit_metadata(fhp);
dput(rdentry);
+ fh_unlock(fhp);
+ iput(rinode); /* truncate the inode here */
out_drop_write:
fh_drop_write(fhp);
--
2.25.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] nfsd: Prevent truncation of an unlinked inode from blocking access to its directory
2021-05-14 3:58 [PATCH] nfsd: Prevent truncation of an unlinked inode from blocking access to its directory Nick Huang
@ 2021-05-14 15:09 ` J. Bruce Fields
2021-05-14 15:46 ` Trond Myklebust
1 sibling, 0 replies; 11+ messages in thread
From: J. Bruce Fields @ 2021-05-14 15:09 UTC (permalink / raw)
To: Nick Huang; +Cc: chuck.lever, linux-nfs, Bing Jing Chang, Robbie Ko
On Fri, May 14, 2021 at 11:58:29AM +0800, Nick Huang wrote:
> From: Yu Hsiang Huang <nickhuang@synology.com>
>
> Truncation of an unlinked inode may take a long time for I/O waiting, and
> it doesn't have to prevent access to the directory. Thus, let truncation
> occur outside the directory's mutex, just like do_unlinkat() does.
Makes sense to me, thanks. I'll queue it up for 5.14.
Just out of curiosity: was this found just by inspection, or were you
hitting this in a real workload? I'd be interested in what it took to
reproduce if so.
--b.
>
> Signed-off-by: Yu Hsiang Huang <nickhuang@synology.com>
> Signed-off-by: Bing Jing Chang <bingjingc@synology.com>
> Signed-off-by: Robbie Ko <robbieko@synology.com>
> ---
> fs/nfsd/vfs.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 15adf1f6ab21..39948f130712 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1859,6 +1859,7 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
> {
> struct dentry *dentry, *rdentry;
> struct inode *dirp;
> + struct inode *rinode;
> __be32 err;
> int host_err;
>
> @@ -1887,6 +1888,8 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
> host_err = -ENOENT;
> goto out_drop_write;
> }
> + rinode = d_inode(rdentry);
> + ihold(rinode);
>
> if (!type)
> type = d_inode(rdentry)->i_mode & S_IFMT;
> @@ -1902,6 +1905,8 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
> if (!host_err)
> host_err = commit_metadata(fhp);
> dput(rdentry);
> + fh_unlock(fhp);
> + iput(rinode); /* truncate the inode here */
>
> out_drop_write:
> fh_drop_write(fhp);
> --
> 2.25.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] nfsd: Prevent truncation of an unlinked inode from blocking access to its directory
2021-05-14 3:58 [PATCH] nfsd: Prevent truncation of an unlinked inode from blocking access to its directory Nick Huang
2021-05-14 15:09 ` J. Bruce Fields
@ 2021-05-14 15:46 ` Trond Myklebust
2021-05-15 7:02 ` Christoph Hellwig
2021-05-18 21:06 ` bfields
1 sibling, 2 replies; 11+ messages in thread
From: Trond Myklebust @ 2021-05-14 15:46 UTC (permalink / raw)
To: bfields, nickhuang, chuck.lever; +Cc: robbieko, bingjingc, linux-nfs
On Fri, 2021-05-14 at 11:58 +0800, Nick Huang wrote:
> From: Yu Hsiang Huang <nickhuang@synology.com>
>
> Truncation of an unlinked inode may take a long time for I/O waiting,
> and
> it doesn't have to prevent access to the directory. Thus, let
> truncation
> occur outside the directory's mutex, just like do_unlinkat() does.
>
> Signed-off-by: Yu Hsiang Huang <nickhuang@synology.com>
> Signed-off-by: Bing Jing Chang <bingjingc@synology.com>
> Signed-off-by: Robbie Ko <robbieko@synology.com>
> ---
> fs/nfsd/vfs.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 15adf1f6ab21..39948f130712 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -1859,6 +1859,7 @@ nfsd_unlink(struct svc_rqst *rqstp, struct
> svc_fh *fhp, int type,
> {
> struct dentry *dentry, *rdentry;
> struct inode *dirp;
> + struct inode *rinode;
> __be32 err;
> int host_err;
>
> @@ -1887,6 +1888,8 @@ nfsd_unlink(struct svc_rqst *rqstp, struct
> svc_fh *fhp, int type,
> host_err = -ENOENT;
> goto out_drop_write;
> }
> + rinode = d_inode(rdentry);
> + ihold(rinode);
>
> if (!type)
> type = d_inode(rdentry)->i_mode & S_IFMT;
> @@ -1902,6 +1905,8 @@ nfsd_unlink(struct svc_rqst *rqstp, struct
> svc_fh *fhp, int type,
> if (!host_err)
> host_err = commit_metadata(fhp);
Why leave the commit_metadata() call under the lock? If you're
concerned about latency, then it makes more sense to call fh_unlock()
before flushing those metadata updates to disk.
This is, BTW, an optimisation that appears to be possible in several
other cases in fs/nfsd/vfs.c.
> dput(rdentry);
> + fh_unlock(fhp);
> + iput(rinode); /* truncate the inode here */
>
> out_drop_write:
> fh_drop_write(fhp);
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] nfsd: Prevent truncation of an unlinked inode from blocking access to its directory
2021-05-14 15:46 ` Trond Myklebust
@ 2021-05-15 7:02 ` Christoph Hellwig
2021-05-17 18:56 ` bfields
2021-05-18 17:53 ` Trond Myklebust
2021-05-18 21:06 ` bfields
1 sibling, 2 replies; 11+ messages in thread
From: Christoph Hellwig @ 2021-05-15 7:02 UTC (permalink / raw)
To: Trond Myklebust
Cc: bfields, nickhuang, chuck.lever, robbieko, bingjingc, linux-nfs
On Fri, May 14, 2021 at 03:46:57PM +0000, Trond Myklebust wrote:
> Why leave the commit_metadata() call under the lock? If you're
> concerned about latency, then it makes more sense to call fh_unlock()
> before flushing those metadata updates to disk.
Also I'm not sure why the extra inode reference is needed. What speaks
against just moving the dput out of the locked section?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] nfsd: Prevent truncation of an unlinked inode from blocking access to its directory
2021-05-15 7:02 ` Christoph Hellwig
@ 2021-05-17 18:56 ` bfields
2021-05-18 7:33 ` Christoph Hellwig
2021-05-18 17:53 ` Trond Myklebust
1 sibling, 1 reply; 11+ messages in thread
From: bfields @ 2021-05-17 18:56 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Trond Myklebust, nickhuang, chuck.lever, robbieko, bingjingc, linux-nfs
On Sat, May 15, 2021 at 08:02:39AM +0100, Christoph Hellwig wrote:
> On Fri, May 14, 2021 at 03:46:57PM +0000, Trond Myklebust wrote:
> > Why leave the commit_metadata() call under the lock? If you're
> > concerned about latency, then it makes more sense to call fh_unlock()
> > before flushing those metadata updates to disk.
>
> Also I'm not sure why the extra inode reference is needed. What speaks
> against just moving the dput out of the locked section?
I don't know. Do you know why do_unlinkat() is doing the same thing?
--b.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] nfsd: Prevent truncation of an unlinked inode from blocking access to its directory
2021-05-17 18:56 ` bfields
@ 2021-05-18 7:33 ` Christoph Hellwig
0 siblings, 0 replies; 11+ messages in thread
From: Christoph Hellwig @ 2021-05-18 7:33 UTC (permalink / raw)
To: bfields
Cc: Christoph Hellwig, Trond Myklebust, nickhuang, chuck.lever,
robbieko, bingjingc, linux-nfs, viro
On Mon, May 17, 2021 at 02:56:59PM -0400, bfields@fieldses.org wrote:
> On Sat, May 15, 2021 at 08:02:39AM +0100, Christoph Hellwig wrote:
> > On Fri, May 14, 2021 at 03:46:57PM +0000, Trond Myklebust wrote:
> > > Why leave the commit_metadata() call under the lock? If you're
> > > concerned about latency, then it makes more sense to call fh_unlock()
> > > before flushing those metadata updates to disk.
> >
> > Also I'm not sure why the extra inode reference is needed. What speaks
> > against just moving the dput out of the locked section?
>
> I don't know. Do you know why do_unlinkat() is doing the same thing?
No. Al, any idea why unlink does the final dput under i_rwsem?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] nfsd: Prevent truncation of an unlinked inode from blocking access to its directory
2021-05-15 7:02 ` Christoph Hellwig
2021-05-17 18:56 ` bfields
@ 2021-05-18 17:53 ` Trond Myklebust
2021-05-18 18:21 ` bfields
1 sibling, 1 reply; 11+ messages in thread
From: Trond Myklebust @ 2021-05-18 17:53 UTC (permalink / raw)
To: hch; +Cc: bfields, robbieko, bingjingc, linux-nfs, nickhuang, chuck.lever
On Sat, 2021-05-15 at 08:02 +0100, Christoph Hellwig wrote:
> On Fri, May 14, 2021 at 03:46:57PM +0000, Trond Myklebust wrote:
> > Why leave the commit_metadata() call under the lock? If you're
> > concerned about latency, then it makes more sense to call
> > fh_unlock()
> > before flushing those metadata updates to disk.
>
> Also I'm not sure why the extra inode reference is needed. What
> speaks
> against just moving the dput out of the locked section?
Isn't the inode reference taken just in order to ensure that the call
to iput_final() (and in particular the call to
truncate_inode_pages_final()) is performed outside the lock?
The dput() is presumably usually not particularly expensive, since the
dentry is just a completely ordinary negative dentry at this point.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] nfsd: Prevent truncation of an unlinked inode from blocking access to its directory
2021-05-18 17:53 ` Trond Myklebust
@ 2021-05-18 18:21 ` bfields
2021-05-18 18:36 ` Trond Myklebust
0 siblings, 1 reply; 11+ messages in thread
From: bfields @ 2021-05-18 18:21 UTC (permalink / raw)
To: Trond Myklebust
Cc: hch, robbieko, bingjingc, linux-nfs, nickhuang, chuck.lever
On Tue, May 18, 2021 at 05:53:47PM +0000, Trond Myklebust wrote:
> On Sat, 2021-05-15 at 08:02 +0100, Christoph Hellwig wrote:
> > On Fri, May 14, 2021 at 03:46:57PM +0000, Trond Myklebust wrote:
> > > Why leave the commit_metadata() call under the lock? If you're
> > > concerned about latency, then it makes more sense to call
> > > fh_unlock()
> > > before flushing those metadata updates to disk.
> >
> > Also I'm not sure why the extra inode reference is needed. What
> > speaks
> > against just moving the dput out of the locked section?
>
> Isn't the inode reference taken just in order to ensure that the call
> to iput_final() (and in particular the call to
> truncate_inode_pages_final()) is performed outside the lock?
>
> The dput() is presumably usually not particularly expensive, since the
> dentry is just a completely ordinary negative dentry at this point.
Right, but why bother with the extra ihold/iput instead of just
postponing the dput?
diff --git a/fs/namei.c b/fs/namei.c
index 79b0ff9b151e..aeed93c9874c 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4084,7 +4084,6 @@ long do_unlinkat(int dfd, struct filename *name)
inode = dentry->d_inode;
if (d_is_negative(dentry))
goto slashes;
- ihold(inode);
error = security_path_unlink(&path, dentry);
if (error)
goto exit2;
@@ -4092,11 +4091,10 @@ long do_unlinkat(int dfd, struct filename *name)
error = vfs_unlink(mnt_userns, path.dentry->d_inode, dentry,
&delegated_inode);
exit2:
- dput(dentry);
}
inode_unlock(path.dentry->d_inode);
- if (inode)
- iput(inode); /* truncate the inode here */
+ if (!IS_ERR(dentry))
+ dput(dentry); /* truncate the inode here */
inode = NULL;
if (delegated_inode) {
error = break_deleg_wait(&delegated_inode);
?
--b.
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] nfsd: Prevent truncation of an unlinked inode from blocking access to its directory
2021-05-18 18:21 ` bfields
@ 2021-05-18 18:36 ` Trond Myklebust
2021-05-18 18:39 ` bfields
0 siblings, 1 reply; 11+ messages in thread
From: Trond Myklebust @ 2021-05-18 18:36 UTC (permalink / raw)
To: bfields; +Cc: robbieko, bingjingc, linux-nfs, hch, nickhuang, chuck.lever
On Tue, 2021-05-18 at 14:21 -0400, bfields@fieldses.org wrote:
> On Tue, May 18, 2021 at 05:53:47PM +0000, Trond Myklebust wrote:
> > On Sat, 2021-05-15 at 08:02 +0100, Christoph Hellwig wrote:
> > > On Fri, May 14, 2021 at 03:46:57PM +0000, Trond Myklebust wrote:
> > > > Why leave the commit_metadata() call under the lock? If you're
> > > > concerned about latency, then it makes more sense to call
> > > > fh_unlock()
> > > > before flushing those metadata updates to disk.
> > >
> > > Also I'm not sure why the extra inode reference is needed. What
> > > speaks
> > > against just moving the dput out of the locked section?
> >
> > Isn't the inode reference taken just in order to ensure that the
> > call
> > to iput_final() (and in particular the call to
> > truncate_inode_pages_final()) is performed outside the lock?
> >
> > The dput() is presumably usually not particularly expensive, since
> > the
> > dentry is just a completely ordinary negative dentry at this point.
>
> Right, but why bother with the extra ihold/iput instead of just
> postponing the dput?
As I said above, the dentry is negative at this point. It doesn't carry
a reference to the inode any more, so that wouldn't defer the inode
truncation.
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 79b0ff9b151e..aeed93c9874c 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4084,7 +4084,6 @@ long do_unlinkat(int dfd, struct filename
> *name)
> inode = dentry->d_inode;
> if (d_is_negative(dentry))
> goto slashes;
> - ihold(inode);
> error = security_path_unlink(&path, dentry);
> if (error)
> goto exit2;
> @@ -4092,11 +4091,10 @@ long do_unlinkat(int dfd, struct filename
> *name)
> error = vfs_unlink(mnt_userns, path.dentry->d_inode,
> dentry,
> &delegated_inode);
> exit2:
> - dput(dentry);
> }
> inode_unlock(path.dentry->d_inode);
> - if (inode)
> - iput(inode); /* truncate the inode here */
> + if (!IS_ERR(dentry))
> + dput(dentry); /* truncate the inode here */
> inode = NULL;
> if (delegated_inode) {
> error = break_deleg_wait(&delegated_inode);
>
> ?
>
> --b.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] nfsd: Prevent truncation of an unlinked inode from blocking access to its directory
2021-05-18 18:36 ` Trond Myklebust
@ 2021-05-18 18:39 ` bfields
0 siblings, 0 replies; 11+ messages in thread
From: bfields @ 2021-05-18 18:39 UTC (permalink / raw)
To: Trond Myklebust
Cc: robbieko, bingjingc, linux-nfs, hch, nickhuang, chuck.lever
On Tue, May 18, 2021 at 06:36:23PM +0000, Trond Myklebust wrote:
> On Tue, 2021-05-18 at 14:21 -0400, bfields@fieldses.org wrote:
> > On Tue, May 18, 2021 at 05:53:47PM +0000, Trond Myklebust wrote:
> > > On Sat, 2021-05-15 at 08:02 +0100, Christoph Hellwig wrote:
> > > > On Fri, May 14, 2021 at 03:46:57PM +0000, Trond Myklebust wrote:
> > > > > Why leave the commit_metadata() call under the lock? If you're
> > > > > concerned about latency, then it makes more sense to call
> > > > > fh_unlock()
> > > > > before flushing those metadata updates to disk.
> > > >
> > > > Also I'm not sure why the extra inode reference is needed. What
> > > > speaks
> > > > against just moving the dput out of the locked section?
> > >
> > > Isn't the inode reference taken just in order to ensure that the
> > > call
> > > to iput_final() (and in particular the call to
> > > truncate_inode_pages_final()) is performed outside the lock?
> > >
> > > The dput() is presumably usually not particularly expensive, since
> > > the
> > > dentry is just a completely ordinary negative dentry at this point.
> >
> > Right, but why bother with the extra ihold/iput instead of just
> > postponing the dput?
>
> As I said above, the dentry is negative at this point. It doesn't carry
> a reference to the inode any more, so that wouldn't defer the inode
> truncation.
Oh, of course, thanks!
--b.
>
> >
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 79b0ff9b151e..aeed93c9874c 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -4084,7 +4084,6 @@ long do_unlinkat(int dfd, struct filename
> > *name)
> > inode = dentry->d_inode;
> > if (d_is_negative(dentry))
> > goto slashes;
> > - ihold(inode);
> > error = security_path_unlink(&path, dentry);
> > if (error)
> > goto exit2;
> > @@ -4092,11 +4091,10 @@ long do_unlinkat(int dfd, struct filename
> > *name)
> > error = vfs_unlink(mnt_userns, path.dentry->d_inode,
> > dentry,
> > &delegated_inode);
> > exit2:
> > - dput(dentry);
> > }
> > inode_unlock(path.dentry->d_inode);
> > - if (inode)
> > - iput(inode); /* truncate the inode here */
> > + if (!IS_ERR(dentry))
> > + dput(dentry); /* truncate the inode here */
> > inode = NULL;
> > if (delegated_inode) {
> > error = break_deleg_wait(&delegated_inode);
> >
> > ?
> >
> > --b.
>
> --
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] nfsd: Prevent truncation of an unlinked inode from blocking access to its directory
2021-05-14 15:46 ` Trond Myklebust
2021-05-15 7:02 ` Christoph Hellwig
@ 2021-05-18 21:06 ` bfields
1 sibling, 0 replies; 11+ messages in thread
From: bfields @ 2021-05-18 21:06 UTC (permalink / raw)
To: Trond Myklebust; +Cc: nickhuang, chuck.lever, robbieko, bingjingc, linux-nfs
On Fri, May 14, 2021 at 03:46:57PM +0000, Trond Myklebust wrote:
> On Fri, 2021-05-14 at 11:58 +0800, Nick Huang wrote:
> > From: Yu Hsiang Huang <nickhuang@synology.com>
> >
> > Truncation of an unlinked inode may take a long time for I/O waiting,
> > and
> > it doesn't have to prevent access to the directory. Thus, let
> > truncation
> > occur outside the directory's mutex, just like do_unlinkat() does.
> >
> > Signed-off-by: Yu Hsiang Huang <nickhuang@synology.com>
> > Signed-off-by: Bing Jing Chang <bingjingc@synology.com>
> > Signed-off-by: Robbie Ko <robbieko@synology.com>
> > ---
> > fs/nfsd/vfs.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index 15adf1f6ab21..39948f130712 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -1859,6 +1859,7 @@ nfsd_unlink(struct svc_rqst *rqstp, struct
> > svc_fh *fhp, int type,
> > {
> > struct dentry *dentry, *rdentry;
> > struct inode *dirp;
> > + struct inode *rinode;
> > __be32 err;
> > int host_err;
> >
> > @@ -1887,6 +1888,8 @@ nfsd_unlink(struct svc_rqst *rqstp, struct
> > svc_fh *fhp, int type,
> > host_err = -ENOENT;
> > goto out_drop_write;
> > }
> > + rinode = d_inode(rdentry);
> > + ihold(rinode);
> >
> > if (!type)
> > type = d_inode(rdentry)->i_mode & S_IFMT;
> > @@ -1902,6 +1905,8 @@ nfsd_unlink(struct svc_rqst *rqstp, struct
> > svc_fh *fhp, int type,
> > if (!host_err)
> > host_err = commit_metadata(fhp);
>
> Why leave the commit_metadata() call under the lock? If you're
> concerned about latency, then it makes more sense to call fh_unlock()
> before flushing those metadata updates to disk.
>
> This is, BTW, an optimisation that appears to be possible in several
> other cases in fs/nfsd/vfs.c.
I'm tentatively applying the original patch plus the following.
Create and rename code are two other places where we have
commit_metadata() calls that could probably be moved out from under
locks but they looked slightly more complicated.
--b.
commit ec79990df716
Author: J. Bruce Fields <bfields@redhat.com>
Date: Fri May 14 18:21:37 2021 -0400
nfsd: move some commit_metadata()s outside the inode lock
The commit may be time-consuming and there's no need to hold the lock
for it.
More of these are possible, these were just some easy ones.
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 39948f130712..d73d3c9126fc 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1613,9 +1613,9 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
host_err = vfs_symlink(&init_user_ns, d_inode(dentry), dnew, path);
err = nfserrno(host_err);
+ fh_unlock(fhp);
if (!err)
err = nfserrno(commit_metadata(fhp));
- fh_unlock(fhp);
fh_drop_write(fhp);
@@ -1680,6 +1680,7 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
if (d_really_is_negative(dold))
goto out_dput;
host_err = vfs_link(dold, &init_user_ns, dirp, dnew, NULL);
+ fh_unlock(ffhp);
if (!host_err) {
err = nfserrno(commit_metadata(ffhp));
if (!err)
@@ -1902,10 +1903,10 @@ nfsd_unlink(struct svc_rqst *rqstp, struct svc_fh *fhp, int type,
host_err = vfs_rmdir(&init_user_ns, dirp, rdentry);
}
+ fh_unlock(fhp);
if (!host_err)
host_err = commit_metadata(fhp);
dput(rdentry);
- fh_unlock(fhp);
iput(rinode); /* truncate the inode here */
out_drop_write:
^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-05-18 21:06 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-14 3:58 [PATCH] nfsd: Prevent truncation of an unlinked inode from blocking access to its directory Nick Huang
2021-05-14 15:09 ` J. Bruce Fields
2021-05-14 15:46 ` Trond Myklebust
2021-05-15 7:02 ` Christoph Hellwig
2021-05-17 18:56 ` bfields
2021-05-18 7:33 ` Christoph Hellwig
2021-05-18 17:53 ` Trond Myklebust
2021-05-18 18:21 ` bfields
2021-05-18 18:36 ` Trond Myklebust
2021-05-18 18:39 ` bfields
2021-05-18 21:06 ` bfields
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.