From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Chinner Subject: Re: [PATCH 4 of 7] Turn the DIO lock_type parameter into a flags field Date: Thu, 2 Nov 2006 13:16:09 +1100 Message-ID: <20061102021609.GS8394166@melbourne.sgi.com> References: <20061101225858.GO8394166@melbourne.sgi.com> <20061102010225.GG31240@think.oraclecorp.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: David Chinner , linux-fsdevel@vger.kernel.org, akpm@osdl.org, zach.brown@oracle.com, Suparna Bhattacharya Return-path: Received: from omx2-ext.sgi.com ([192.48.171.19]:55951 "EHLO omx2.sgi.com") by vger.kernel.org with ESMTP id S1752527AbWKBCQk (ORCPT ); Wed, 1 Nov 2006 21:16:40 -0500 To: Chris Mason Content-Disposition: inline In-Reply-To: <20061102010225.GG31240@think.oraclecorp.com> Sender: linux-fsdevel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On Wed, Nov 01, 2006 at 08:02:25PM -0500, Chris Mason wrote: > On Thu, Nov 02, 2006 at 09:58:58AM +1100, David Chinner wrote: > [ ...] > > Thanks for the review, I'll incorporate these suggestions into the next > patch set. > > > > + > > > + /* > > > + * the placeholder code does filemap_write_and_wait, so if we > > > + * aren't using placeholders we have to do it here > > > + */ > > > + if (!(dio->flags & DIO_PLACEHOLDERS) && end > offset) { > > > + struct address_space *mapping = iocb->ki_filp->f_mapping; > > > retval = filemap_write_and_wait_range(mapping, offset, end - 1); > > > if (retval) > > > goto out; > > > > So that means XFS will now do three cache flushes on write (one in > > xfs_write(), one in generic_file_direct_IO() and yet another here.... > > I removed the flush before starting the IO in generic_file_direct_IO > because that is now done by the placeholders. When placeholders aren't > used, we need the flush to match the old behavior. OK - given that XFS does a flush-and-invalidate (with the i_mutex held) before calling the generic direct I/O code, do we have grounds for another flag here as it is not needed for XFS? (FWIW, XFS's fs_flushinval_pages() could be made smarter now that we have range based functions that can be used....) > I added the flush in generic_file_direct_IO after calling the direct_IO > func because invalidate_page_range2 requires the data to be flushed > first. Otherwise it fails to free a dirty page with dirty buffers and > spits out -EIO. This only happens when mapping->nrpages > 0, so it > should be very low cost in the common case. *nod*. XFS does it's flush-inval does on the same criteria. > It's messy, I'll try to refactor things a bit to make it cleaner (same > goes for i_mutex drop/lock). True, but it is a lot less messy than the current code ;) Cheers, Dave. -- Dave Chinner Principal Engineer SGI Australian Software Group