From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com ([209.132.183.28]:48566 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752500AbdGSLBw (ORCPT ); Wed, 19 Jul 2017 07:01:52 -0400 Date: Wed, 19 Jul 2017 13:01:48 +0200 From: Lukas Czerner To: Jan Kara Cc: linux-fsdevel@vger.kernel.org, viro@zeniv.linux.org.uk, hch@infradead.org, Jeff Moyer Subject: Re: [PATCH v4] fs: Fix page cache inconsistency when mixing buffered and AIO DIO Message-ID: <20170719110148.w6hq5gemyimncjs3@localhost.localdomain> References: <1500380368-31661-1-git-send-email-lczerner@redhat.com> <1500454124-32205-1-git-send-email-lczerner@redhat.com> <20170719092637.GA14679@quack2.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170719092637.GA14679@quack2.suse.cz> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Wed, Jul 19, 2017 at 11:26:37AM +0200, Jan Kara wrote: > in question. This ensures that after the completion the pages > > in the written area are either unmapped, or populated with up-to-date > > data. Also do the same for the iomap case which uses > > iomap_dio_complete() instead. > > > > This has a side effect of deferring the completion to a process context > > for every AIO DIO that happens on inode that has pages mapped. However > > since the consensus is that this is ill-advised practice the performance > > implication should not be a problem. > > > > This was based on proposal from Jeff Moyer, thanks! > > > > Signed-off-by: Lukas Czerner > > Cc: Jeff Moyer > > --- > > v2: Remove leftover ret variable from invalidate call in iomap_dio_complete > > v3: Do not invalidate in case of error. Add some coments > > v4: Remove unnecessary variable, remove unnecessary inner braces > > Looks good to me now, just two style nits below. You can add: > > Reviewed-by: Jan Kara > > > diff --git a/fs/direct-io.c b/fs/direct-io.c > > index 08cf278..efd3246 100644 > > --- a/fs/direct-io.c > > +++ b/fs/direct-io.c > > @@ -258,6 +258,12 @@ static ssize_t dio_complete(struct dio *dio, ssize_t ret, bool is_async) > > if (ret == 0) > > ret = transferred; > > > > + if ((ret > 0) && > > + (dio->op == REQ_OP_WRITE && dio->inode->i_mapping->nrpages)) > > Heh, you seem to love braces. The general rule is that braces should be > around bit-ops (as there people find the priority unclear and also it is > too easy to forget to add those braces when negating the condition) but not > around comparison or such. I.e. the above would be: > > if (ret > 0 && dio->op == REQ_OP_WRITE && > dio->inode->i_mapping->nrpages) :D sure, I'll resend. > > ... > > > + if (dio->is_async && iov_iter_rw(iter) == WRITE) { > > + retval = 0; > > + if ((iocb->ki_filp->f_flags & O_DSYNC) || > > + IS_SYNC(iocb->ki_filp->f_mapping->host)) > > + retval = dio_set_defer_completion(dio); > > + else if (!dio->inode->i_sb->s_dio_done_wq) > > + /* > > + * In case of AIO write racing with buffered read we > > + * need to defer completion. We can't decide this now, > > + * however the workqueue needs to be initialized here. > > + */ > > + retval = sb_init_dio_done_wq(dio->inode->i_sb); > > Curly braces here please. When command block is multi-line we enforce those > despite it is only a single statement and thus they are not necessary > strictly speaking. Thanks! ok. Thanks! -Lukas > > Honza > -- > Jan Kara > SUSE Labs, CR