Re: [PATCH v4] fs: Fix page cache inconsistency when mixing buffered and AIO DIO

[Lukas Czerner <lczerner@redhat.com>]

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 <lczerner@redhat.com>
> > Cc: Jeff Moyer <jmoyer@redhat.com>
> > ---
> > 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 <jack@suse.cz>
> 
> > 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 <jack@suse.com>
> SUSE Labs, CR