From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt0-f174.google.com ([209.85.216.174]:32990 "EHLO mail-qt0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S940888AbdEZQ6K (ORCPT ); Fri, 26 May 2017 12:58:10 -0400 Received: by mail-qt0-f174.google.com with SMTP id t26so12984780qtg.0 for ; Fri, 26 May 2017 09:58:09 -0700 (PDT) Message-ID: <1495817887.4299.6.camel@redhat.com> Subject: Re: [PATCH 11/13] orangefs: lock inode during fsync From: Jeff Layton To: martin@omnibond.com Cc: hubcap@omnibond.com, linux-fsdevel@vger.kernel.org Date: Fri, 26 May 2017 12:58:07 -0400 In-Reply-To: <20170526162150.GA17804@pamir.hexium.org> References: <1495447141-12216-1-git-send-email-martin@omnibond.com> <1495447141-12216-12-git-send-email-martin@omnibond.com> <1495727937.2928.3.camel@redhat.com> <20170526162150.GA17804@pamir.hexium.org> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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 > > > --- > > > 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 > > 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