| Message ID | 20190408145942.6448-1-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] cifs: remove superfluous inode_lock in cifs_{strict_}fsync | expand |
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 >
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
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
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
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; }
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