| Message ID | 1495447141-12216-12-git-send-email-martin@omnibond.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2017-05-22 at 05:58 -0400, Martin Brandenburg wrote: > Signed-off-by: Martin Brandenburg <martin@omnibond.com> > --- > fs/orangefs/file.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c > index cd126dd..f8536a7 100644 > --- a/fs/orangefs/file.c > +++ b/fs/orangefs/file.c > @@ -652,7 +652,9 @@ static int orangefs_fsync(struct file *file, > struct orangefs_kernel_op_s *new_op = NULL; > > /* required call */ > + inode_lock(file_inode(file)); > filemap_write_and_wait_range(file->f_mapping, start, end); > + inode_unlock(file_inode(file)); > > new_op = op_alloc(ORANGEFS_VFS_OP_FSYNC); > if (!new_op) Why? You're just writing back the cached file data here. There's no reason to lock the inode for that, AFAICS.
On Thu, May 25, 2017 at 11:58:57AM -0400, Jeff Layton wrote: > On Mon, 2017-05-22 at 05:58 -0400, Martin Brandenburg wrote: > > Signed-off-by: Martin Brandenburg <martin@omnibond.com> > > --- > > fs/orangefs/file.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c > > index cd126dd..f8536a7 100644 > > --- a/fs/orangefs/file.c > > +++ b/fs/orangefs/file.c > > @@ -652,7 +652,9 @@ static int orangefs_fsync(struct file *file, > > struct orangefs_kernel_op_s *new_op = NULL; > > > > /* required call */ > > + inode_lock(file_inode(file)); > > filemap_write_and_wait_range(file->f_mapping, start, end); > > + inode_unlock(file_inode(file)); > > > > new_op = op_alloc(ORANGEFS_VFS_OP_FSYNC); > > if (!new_op) > > Why? You're just writing back the cached file data here. There's no > reason to lock the inode for that, AFAICS. > > -- > Jeff Layton <jlayton@redhat.com> Because FUSE does. Now I see FUSE needs it for fuse_sync_writes. /* * Wait for all pending writepages on the inode to finish. * * This is currently done by blocking further writes with FUSE_NOWRITE * and waiting for all sent writes to complete. * * This must be called under i_mutex, otherwise the FUSE_NOWRITE usage * could conflict with truncation. */ static void fuse_sync_writes(struct inode *inode) I think OrangeFS is okay because writepage does not return until the write has completed. Does that sound right? I'm not sure about the truncate conflict. Truncates are sent to the server immediately. Can a pending write show past the end of the file show up later? I don't handle that at all. Martin
On Fri, 2017-05-26 at 12:21 -0400, martin@omnibond.com wrote: > On Thu, May 25, 2017 at 11:58:57AM -0400, Jeff Layton wrote: > > On Mon, 2017-05-22 at 05:58 -0400, Martin Brandenburg wrote: > > > Signed-off-by: Martin Brandenburg <martin@omnibond.com> > > > --- > > > fs/orangefs/file.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c > > > index cd126dd..f8536a7 100644 > > > --- a/fs/orangefs/file.c > > > +++ b/fs/orangefs/file.c > > > @@ -652,7 +652,9 @@ static int orangefs_fsync(struct file *file, > > > struct orangefs_kernel_op_s *new_op = NULL; > > > > > > /* required call */ > > > + inode_lock(file_inode(file)); > > > filemap_write_and_wait_range(file->f_mapping, start, end); > > > + inode_unlock(file_inode(file)); > > > > > > new_op = op_alloc(ORANGEFS_VFS_OP_FSYNC); > > > if (!new_op) > > > > Why? You're just writing back the cached file data here. There's no > > reason to lock the inode for that, AFAICS. > > > > -- > > Jeff Layton <jlayton@redhat.com> > > Because FUSE does. Now I see FUSE needs it for fuse_sync_writes. > > /* > * Wait for all pending writepages on the inode to finish. > * > * This is currently done by blocking further writes with FUSE_NOWRITE > * and waiting for all sent writes to complete. > * > * This must be called under i_mutex, otherwise the FUSE_NOWRITE usage > * could conflict with truncation. > */ > static void fuse_sync_writes(struct inode *inode) > > I think OrangeFS is okay because writepage does not return until the > write has completed. Does that sound right? > I think so. > I'm not sure about the truncate conflict. Truncates are sent to the > server immediately. Can a pending write show past the end of the file > show up later? I don't handle that at all. > IIUC, the danger there is generally that the write and truncate could get reordered. If they're both synchronous though then I don't see how that could happen -- Jeff Layton <jlayton@redhat.com>
diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c index cd126dd..f8536a7 100644 --- a/fs/orangefs/file.c +++ b/fs/orangefs/file.c @@ -652,7 +652,9 @@ static int orangefs_fsync(struct file *file, struct orangefs_kernel_op_s *new_op = NULL; /* required call */ + inode_lock(file_inode(file)); filemap_write_and_wait_range(file->f_mapping, start, end); + inode_unlock(file_inode(file)); new_op = op_alloc(ORANGEFS_VFS_OP_FSYNC); if (!new_op)
Signed-off-by: Martin Brandenburg <martin@omnibond.com> --- fs/orangefs/file.c | 2 ++ 1 file changed, 2 insertions(+)