From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-fsdevel-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:41542 "EHLO mx1.redhat.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1751448AbdGSIm4 (ORCPT <rfc822;linux-fsdevel@vger.kernel.org>);
        Wed, 19 Jul 2017 04:42:56 -0400
Date: Wed, 19 Jul 2017 10:42:52 +0200
From: Lukas Czerner <lczerner@redhat.com>
To: Christoph Hellwig <hch@infradead.org>
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: <20170719084252.fxwvckhyk6b2e5ik@localhost.localdomain>
References: <1500046823-25256-1-git-send-email-lczerner@redhat.com>
 <1500380368-31661-1-git-send-email-lczerner@redhat.com>
 <20170718134420.GA9411@infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170718134420.GA9411@infradead.org>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

On Tue, Jul 18, 2017 at 06:44:20AM -0700, Christoph Hellwig wrote:
> ->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.

As Jan mentioned and I agree it's worth initializing it only when
actually needed.

> 
> > 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.

Right, I did not use it later in the fucntion because it would be
confusing (we're changing iocb->ki_pos). So I'll just remove the
variable.

> 
> > +	struct inode *inode = file_inode(iocb->ki_filp);
> >  	ssize_t ret;
> >  
> > +	if ((!dio->error) &&
> 
> no need for the inner braces.

ok


Thanks!
-Lukas