* [RFC PATCH] cifs: remove superfluous inode_lock in cifs_{strict_}fsync
@ 2019-04-08 14:59 Jeff Layton
2019-04-08 15:29 ` Steve French
0 siblings, 1 reply; 5+ messages in thread
From: Jeff Layton @ 2019-04-08 14:59 UTC (permalink / raw)
To: smfrench; +Cc: linux-cifs
Originally, filemap_write_and_wait took the i_mutex internally, but
commit 02c24a82187d pushed the mutex acquisition into the individual
fsync routines, leaving it up to the subsystem maintainers to remove
it if it wasn't needed.
For cifs, I see no reason to take the inode_lock here. All of the
operations inside that lock are protected in other ways.
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
fs/cifs/file.c | 5 -----
1 file changed, 5 deletions(-)
Steve, would you or someone else be able to test this?
I'm proposing a similar patch for kcephfs. I don't have an appropriate
test rig at the moment, and I was hoping someone more involved with
cifs.ko development these days may be able to do so.
Thanks,
Jeff
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 89006e044973..c4b84fd423c3 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -2424,7 +2424,6 @@ int cifs_strict_fsync(struct file *file, loff_t start, loff_t end,
rc = file_write_and_wait_range(file, start, end);
if (rc)
return rc;
- inode_lock(inode);
xid = get_xid();
@@ -2449,7 +2448,6 @@ int cifs_strict_fsync(struct file *file, loff_t start, loff_t end,
}
free_xid(xid);
- inode_unlock(inode);
return rc;
}
@@ -2461,12 +2459,10 @@ int cifs_fsync(struct file *file, loff_t start, loff_t end, int datasync)
struct TCP_Server_Info *server;
struct cifsFileInfo *smbfile = file->private_data;
struct cifs_sb_info *cifs_sb = CIFS_FILE_SB(file);
- struct inode *inode = file->f_mapping->host;
rc = file_write_and_wait_range(file, start, end);
if (rc)
return rc;
- inode_lock(inode);
xid = get_xid();
@@ -2483,7 +2479,6 @@ int cifs_fsync(struct file *file, loff_t start, loff_t end, int datasync)
}
free_xid(xid);
- inode_unlock(inode);
return rc;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC PATCH] cifs: remove superfluous inode_lock in cifs_{strict_}fsync
2019-04-08 14:59 [RFC PATCH] cifs: remove superfluous inode_lock in cifs_{strict_}fsync Jeff Layton
@ 2019-04-08 15:29 ` Steve French
2019-04-08 15:41 ` Steve French
0 siblings, 1 reply; 5+ messages in thread
From: Steve French @ 2019-04-08 15:29 UTC (permalink / raw)
To: Jeff Layton; +Cc: CIFS
Kicked off automated tests with this patch included - see
http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/2/builds/184
(I will kick off other test buckets later)
On Mon, Apr 8, 2019 at 9:59 AM Jeff Layton <jlayton@kernel.org> wrote:
>
> Originally, filemap_write_and_wait took the i_mutex internally, but
> commit 02c24a82187d pushed the mutex acquisition into the individual
> fsync routines, leaving it up to the subsystem maintainers to remove
> it if it wasn't needed.
>
> For cifs, I see no reason to take the inode_lock here. All of the
> operations inside that lock are protected in other ways.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> fs/cifs/file.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> Steve, would you or someone else be able to test this?
>
> I'm proposing a similar patch for kcephfs. I don't have an appropriate
> test rig at the moment, and I was hoping someone more involved with
> cifs.ko development these days may be able to do so.
>
> Thanks,
> Jeff
>
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index 89006e044973..c4b84fd423c3 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -2424,7 +2424,6 @@ int cifs_strict_fsync(struct file *file, loff_t start, loff_t end,
> rc = file_write_and_wait_range(file, start, end);
> if (rc)
> return rc;
> - inode_lock(inode);
>
> xid = get_xid();
>
> @@ -2449,7 +2448,6 @@ int cifs_strict_fsync(struct file *file, loff_t start, loff_t end,
> }
>
> free_xid(xid);
> - inode_unlock(inode);
> return rc;
> }
>
> @@ -2461,12 +2459,10 @@ int cifs_fsync(struct file *file, loff_t start, loff_t end, int datasync)
> struct TCP_Server_Info *server;
> struct cifsFileInfo *smbfile = file->private_data;
> struct cifs_sb_info *cifs_sb = CIFS_FILE_SB(file);
> - struct inode *inode = file->f_mapping->host;
>
> rc = file_write_and_wait_range(file, start, end);
> if (rc)
> return rc;
> - inode_lock(inode);
>
> xid = get_xid();
>
> @@ -2483,7 +2479,6 @@ int cifs_fsync(struct file *file, loff_t start, loff_t end, int datasync)
> }
>
> free_xid(xid);
> - inode_unlock(inode);
> return rc;
> }
>
> --
> 2.20.1
>
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH] cifs: remove superfluous inode_lock in cifs_{strict_}fsync
2019-04-08 15:29 ` Steve French
@ 2019-04-08 15:41 ` Steve French
2019-04-08 23:08 ` Pavel Shilovsky
0 siblings, 1 reply; 5+ messages in thread
From: Steve French @ 2019-04-08 15:41 UTC (permalink / raw)
To: Jeff Layton; +Cc: CIFS
Restarted the automated tests after updating for-next and added this patch ontop
http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/2/builds/185
On Mon, Apr 8, 2019 at 10:29 AM Steve French <smfrench@gmail.com> wrote:
>
> Kicked off automated tests with this patch included - see
> http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/2/builds/184
> (I will kick off other test buckets later)
>
> On Mon, Apr 8, 2019 at 9:59 AM Jeff Layton <jlayton@kernel.org> wrote:
> >
> > Originally, filemap_write_and_wait took the i_mutex internally, but
> > commit 02c24a82187d pushed the mutex acquisition into the individual
> > fsync routines, leaving it up to the subsystem maintainers to remove
> > it if it wasn't needed.
> >
> > For cifs, I see no reason to take the inode_lock here. All of the
> > operations inside that lock are protected in other ways.
> >
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > fs/cifs/file.c | 5 -----
> > 1 file changed, 5 deletions(-)
> >
> > Steve, would you or someone else be able to test this?
> >
> > I'm proposing a similar patch for kcephfs. I don't have an appropriate
> > test rig at the moment, and I was hoping someone more involved with
> > cifs.ko development these days may be able to do so.
> >
> > Thanks,
> > Jeff
> >
> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > index 89006e044973..c4b84fd423c3 100644
> > --- a/fs/cifs/file.c
> > +++ b/fs/cifs/file.c
> > @@ -2424,7 +2424,6 @@ int cifs_strict_fsync(struct file *file, loff_t start, loff_t end,
> > rc = file_write_and_wait_range(file, start, end);
> > if (rc)
> > return rc;
> > - inode_lock(inode);
> >
> > xid = get_xid();
> >
> > @@ -2449,7 +2448,6 @@ int cifs_strict_fsync(struct file *file, loff_t start, loff_t end,
> > }
> >
> > free_xid(xid);
> > - inode_unlock(inode);
> > return rc;
> > }
> >
> > @@ -2461,12 +2459,10 @@ int cifs_fsync(struct file *file, loff_t start, loff_t end, int datasync)
> > struct TCP_Server_Info *server;
> > struct cifsFileInfo *smbfile = file->private_data;
> > struct cifs_sb_info *cifs_sb = CIFS_FILE_SB(file);
> > - struct inode *inode = file->f_mapping->host;
> >
> > rc = file_write_and_wait_range(file, start, end);
> > if (rc)
> > return rc;
> > - inode_lock(inode);
> >
> > xid = get_xid();
> >
> > @@ -2483,7 +2479,6 @@ int cifs_fsync(struct file *file, loff_t start, loff_t end, int datasync)
> > }
> >
> > free_xid(xid);
> > - inode_unlock(inode);
> > return rc;
> > }
> >
> > --
> > 2.20.1
> >
>
>
> --
> Thanks,
>
> Steve
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH] cifs: remove superfluous inode_lock in cifs_{strict_}fsync
2019-04-08 15:41 ` Steve French
@ 2019-04-08 23:08 ` Pavel Shilovsky
2019-04-09 0:29 ` Steve French
0 siblings, 1 reply; 5+ messages in thread
From: Pavel Shilovsky @ 2019-04-08 23:08 UTC (permalink / raw)
To: Steve French; +Cc: Jeff Layton, CIFS
Acked-by: Pavel Shilovsky <pshilov@microsoft.com>
--
Best regards,
Pavel Shilovsky
пн, 8 апр. 2019 г. в 09:06, Steve French <smfrench@gmail.com>:
>
> Restarted the automated tests after updating for-next and added this patch ontop
>
> http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/2/builds/185
>
> On Mon, Apr 8, 2019 at 10:29 AM Steve French <smfrench@gmail.com> wrote:
> >
> > Kicked off automated tests with this patch included - see
> > http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/2/builds/184
> > (I will kick off other test buckets later)
> >
> > On Mon, Apr 8, 2019 at 9:59 AM Jeff Layton <jlayton@kernel.org> wrote:
> > >
> > > Originally, filemap_write_and_wait took the i_mutex internally, but
> > > commit 02c24a82187d pushed the mutex acquisition into the individual
> > > fsync routines, leaving it up to the subsystem maintainers to remove
> > > it if it wasn't needed.
> > >
> > > For cifs, I see no reason to take the inode_lock here. All of the
> > > operations inside that lock are protected in other ways.
> > >
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > > fs/cifs/file.c | 5 -----
> > > 1 file changed, 5 deletions(-)
> > >
> > > Steve, would you or someone else be able to test this?
> > >
> > > I'm proposing a similar patch for kcephfs. I don't have an appropriate
> > > test rig at the moment, and I was hoping someone more involved with
> > > cifs.ko development these days may be able to do so.
> > >
> > > Thanks,
> > > Jeff
> > >
> > > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > > index 89006e044973..c4b84fd423c3 100644
> > > --- a/fs/cifs/file.c
> > > +++ b/fs/cifs/file.c
> > > @@ -2424,7 +2424,6 @@ int cifs_strict_fsync(struct file *file, loff_t start, loff_t end,
> > > rc = file_write_and_wait_range(file, start, end);
> > > if (rc)
> > > return rc;
> > > - inode_lock(inode);
> > >
> > > xid = get_xid();
> > >
> > > @@ -2449,7 +2448,6 @@ int cifs_strict_fsync(struct file *file, loff_t start, loff_t end,
> > > }
> > >
> > > free_xid(xid);
> > > - inode_unlock(inode);
> > > return rc;
> > > }
> > >
> > > @@ -2461,12 +2459,10 @@ int cifs_fsync(struct file *file, loff_t start, loff_t end, int datasync)
> > > struct TCP_Server_Info *server;
> > > struct cifsFileInfo *smbfile = file->private_data;
> > > struct cifs_sb_info *cifs_sb = CIFS_FILE_SB(file);
> > > - struct inode *inode = file->f_mapping->host;
> > >
> > > rc = file_write_and_wait_range(file, start, end);
> > > if (rc)
> > > return rc;
> > > - inode_lock(inode);
> > >
> > > xid = get_xid();
> > >
> > > @@ -2483,7 +2479,6 @@ int cifs_fsync(struct file *file, loff_t start, loff_t end, int datasync)
> > > }
> > >
> > > free_xid(xid);
> > > - inode_unlock(inode);
> > > return rc;
> > > }
> > >
> > > --
> > > 2.20.1
> > >
> >
> >
> > --
> > Thanks,
> >
> > Steve
>
>
>
> --
> Thanks,
>
> Steve
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH] cifs: remove superfluous inode_lock in cifs_{strict_}fsync
2019-04-08 23:08 ` Pavel Shilovsky
@ 2019-04-09 0:29 ` Steve French
0 siblings, 0 replies; 5+ messages in thread
From: Steve French @ 2019-04-09 0:29 UTC (permalink / raw)
To: Pavel Shilovsky; +Cc: Jeff Layton, CIFS
It passed automated testing (the most recent test bucket run was
http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/4/builds/141
but it also passed the other test group
http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/2/builds/185)
On Mon, Apr 8, 2019 at 6:09 PM Pavel Shilovsky <piastryyy@gmail.com> wrote:
>
> Acked-by: Pavel Shilovsky <pshilov@microsoft.com>
>
> --
> Best regards,
> Pavel Shilovsky
>
> пн, 8 апр. 2019 г. в 09:06, Steve French <smfrench@gmail.com>:
> >
> > Restarted the automated tests after updating for-next and added this patch ontop
> >
> > http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/2/builds/185
> >
> > On Mon, Apr 8, 2019 at 10:29 AM Steve French <smfrench@gmail.com> wrote:
> > >
> > > Kicked off automated tests with this patch included - see
> > > http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/2/builds/184
> > > (I will kick off other test buckets later)
> > >
> > > On Mon, Apr 8, 2019 at 9:59 AM Jeff Layton <jlayton@kernel.org> wrote:
> > > >
> > > > Originally, filemap_write_and_wait took the i_mutex internally, but
> > > > commit 02c24a82187d pushed the mutex acquisition into the individual
> > > > fsync routines, leaving it up to the subsystem maintainers to remove
> > > > it if it wasn't needed.
> > > >
> > > > For cifs, I see no reason to take the inode_lock here. All of the
> > > > operations inside that lock are protected in other ways.
> > > >
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > ---
> > > > fs/cifs/file.c | 5 -----
> > > > 1 file changed, 5 deletions(-)
> > > >
> > > > Steve, would you or someone else be able to test this?
> > > >
> > > > I'm proposing a similar patch for kcephfs. I don't have an appropriate
> > > > test rig at the moment, and I was hoping someone more involved with
> > > > cifs.ko development these days may be able to do so.
> > > >
> > > > Thanks,
> > > > Jeff
> > > >
> > > > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > > > index 89006e044973..c4b84fd423c3 100644
> > > > --- a/fs/cifs/file.c
> > > > +++ b/fs/cifs/file.c
> > > > @@ -2424,7 +2424,6 @@ int cifs_strict_fsync(struct file *file, loff_t start, loff_t end,
> > > > rc = file_write_and_wait_range(file, start, end);
> > > > if (rc)
> > > > return rc;
> > > > - inode_lock(inode);
> > > >
> > > > xid = get_xid();
> > > >
> > > > @@ -2449,7 +2448,6 @@ int cifs_strict_fsync(struct file *file, loff_t start, loff_t end,
> > > > }
> > > >
> > > > free_xid(xid);
> > > > - inode_unlock(inode);
> > > > return rc;
> > > > }
> > > >
> > > > @@ -2461,12 +2459,10 @@ int cifs_fsync(struct file *file, loff_t start, loff_t end, int datasync)
> > > > struct TCP_Server_Info *server;
> > > > struct cifsFileInfo *smbfile = file->private_data;
> > > > struct cifs_sb_info *cifs_sb = CIFS_FILE_SB(file);
> > > > - struct inode *inode = file->f_mapping->host;
> > > >
> > > > rc = file_write_and_wait_range(file, start, end);
> > > > if (rc)
> > > > return rc;
> > > > - inode_lock(inode);
> > > >
> > > > xid = get_xid();
> > > >
> > > > @@ -2483,7 +2479,6 @@ int cifs_fsync(struct file *file, loff_t start, loff_t end, int datasync)
> > > > }
> > > >
> > > > free_xid(xid);
> > > > - inode_unlock(inode);
> > > > return rc;
> > > > }
> > > >
> > > > --
> > > > 2.20.1
> > > >
> > >
> > >
> > > --
> > > Thanks,
> > >
> > > Steve
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve
--
Thanks,
Steve
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-04-09 0:29 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-08 14:59 [RFC PATCH] cifs: remove superfluous inode_lock in cifs_{strict_}fsync Jeff Layton
2019-04-08 15:29 ` Steve French
2019-04-08 15:41 ` Steve French
2019-04-08 23:08 ` Pavel Shilovsky
2019-04-09 0:29 ` Steve French
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).