From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-fsdevel-owner@vger.kernel.org>
Received: from bombadil.infradead.org ([65.50.211.133]:50733 "EHLO
        bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751399AbdGRNoV (ORCPT
        <rfc822;linux-fsdevel@vger.kernel.org>);
        Tue, 18 Jul 2017 09:44:21 -0400
Date: Tue, 18 Jul 2017 06:44:20 -0700
From: Christoph Hellwig <hch@infradead.org>
To: Lukas Czerner <lczerner@redhat.com>
Cc: linux-fsdevel@vger.kernel.org, viro@zeniv.linux.org.uk,
        jack@suse.cz, Jeff Moyer <jmoyer@redhat.com>
Subject: Re: [PATCH v3] fs: Fix page cache inconsistency when mixing buffered
 and AIO DIO
Message-ID: <20170718134420.GA9411@infradead.org>
References: <1500046823-25256-1-git-send-email-lczerner@redhat.com>
 <1500380368-31661-1-git-send-email-lczerner@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1500380368-31661-1-git-send-email-lczerner@redhat.com>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

> +	if ((ret > 0) &&

No need for the braces here.

> +	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);

So now we initialize the workqueue on the first aio write.  Maybe we
should just always initialize it?  Especially given that the cost of
a workqueue is rather cheap.  I also don't really understand why
we even need the workqueue per-superblock instead of global.

> index 1732228..2f8dbf9 100644
> --- a/fs/iomap.c
> +++ b/fs/iomap.c
> @@ -713,8 +713,16 @@ struct iomap_dio {
>  static ssize_t iomap_dio_complete(struct iomap_dio *dio)
>  {
>  	struct kiocb *iocb = dio->iocb;
> +	loff_t offset = iocb->ki_pos;

If you introduce this variable please also use it later in the function
instead of iocb->ki_pos.  OR remove the variable, which would be fine
with me as well.

> +	struct inode *inode = file_inode(iocb->ki_filp);
>  	ssize_t ret;
>  
> +	if ((!dio->error) &&

no need for the inner braces.