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
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
