public inbox for linux-xfs@vger.kernel.org
 help / color / mirror / Atom feed
From: Alex Elder <aelder@sgi.com>
To: Dave Chinner <david@fromorbit.com>
Cc: Christoph Hellwig <hch@infradead.org>, xfs@oss.sgi.com
Subject: Re: [PATCH 4/8] xfs: introduce xfs_rw_lock() helpers for locking the inode
Date: Tue, 11 Jan 2011 15:36:03 -0600	[thread overview]
Message-ID: <1294781763.3115.9.camel@doink> (raw)
In-Reply-To: <20110111210250.GL28803@dastard>

On Wed, 2011-01-12 at 08:02 +1100, Dave Chinner wrote:
> On Tue, Jan 11, 2011 at 12:36:27PM -0500, Christoph Hellwig wrote:
> > On Tue, Jan 11, 2011 at 10:37:44AM +1100, Dave Chinner wrote:
> > > +/*
> > > + * xfs_file_splice_write() does not use xfs_rw_ilock() because
> > > + * generic_file_splice_write() takes the i_mutex itself. This, in theory,

. . .

> > > +	xfs_rw_iunlock(ip, XFS_ILOCK_EXCL);
> > 
> > While using the xfs_rw_iunlock here actually is correct I think it's
> 
> Argh! I thought I reverted all the changes to
> xfs_file_splice_write().
> 
> > highly confusing, given that we didn't use it to take the ilock.
> 
> It definitely held the ilock around that size update before this
> series. ;)
> 
> > What
> > about just leaving all the code in xfs_file_splice_write as-is and not
> > touching it at all?
> 
> Updated version below.

Your explanation before and this update addressed all
my substantive issues.  I have one other thing (which
is essentially a style issue), which I'll still mention
below, but I think your next patch may make it moot
anyway...

In any case:

Reviewed-by: Alex Elder <aelder@sgi.com>

. . .
> @@ -654,16 +690,20 @@ start:
>  				mp->m_rtdev_targp : mp->m_ddev_targp;
>  
>  		if ((pos & target->bt_smask) || (count & target->bt_smask)) {
> -			xfs_iunlock(ip, XFS_ILOCK_EXCL|iolock);
> +			xfs_rw_iunlock(ip, XFS_ILOCK_EXCL|iolock);
>  			return XFS_ERROR(-EINVAL);
>  		}
>  
> -		if (!need_i_mutex && (mapping->nrpages || pos > ip->i_size)) {
> -			xfs_iunlock(ip, XFS_ILOCK_EXCL|iolock);
> +		/*
> +		 * For direct I/O, if there are cached pages or we're extending
> +		 * the file, we need IOLOCK_EXCL until we're sure the bytes at
> +		 * the new EOF have been zeroed and/or the cached pages are
> +		 * flushed out.  Upgrade the I/O lock and start again.
> +		 */
> +		if (iolock != XFS_IOLOCK_EXCL &&

I would prefer:   if (iolock == XFS_IOLOCK_SHARED)

> +		    (mapping->nrpages || pos > ip->i_size)) {
> +			xfs_rw_iunlock(ip, XFS_ILOCK_EXCL|iolock);

and (maybe) this could be             XFS_ILOCK_EXCL|XFS_IOLOCK_SHARED);

>  			iolock = XFS_IOLOCK_EXCL;
> -			need_i_mutex = 1;
> -			mutex_lock(&inode->i_mutex);
> -			xfs_ilock(ip, XFS_ILOCK_EXCL|iolock);
>  			goto start;
>  		}
>  	}

. . .

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

  parent reply	other threads:[~2011-01-11 21:34 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-01-10 23:37 [PATCH 0/8] xfs: prevent corruption due to overlapping AIO DIO V4 Dave Chinner
2011-01-10 23:37 ` [PATCH 1/8] xfs: ensure sync write errors are returned Dave Chinner
2011-01-10 23:37 ` [PATCH 2/8] xfs: factor common post-write isize handling code Dave Chinner
2011-01-10 23:37 ` [PATCH 4/8] xfs: introduce xfs_rw_lock() helpers for locking the inode Dave Chinner
2011-01-11 17:36   ` Christoph Hellwig
2011-01-11 21:02     ` Dave Chinner
2011-01-11 21:03       ` Christoph Hellwig
2011-01-11 21:36       ` Alex Elder [this message]
2011-01-10 23:37 ` [PATCH 5/8] xfs: split direct IO write path from xfs_file_aio_write Dave Chinner
2011-01-11 21:44   ` Alex Elder
2011-01-10 23:37 ` [PATCH 6/8] xfs: split buffered " Dave Chinner
2011-01-10 23:37 ` [PATCH 7/8] xfs: factor common write setup code Dave Chinner
2011-01-10 23:37 ` [PATCH 8/8] xfs: serialise unaligned direct IOs Dave Chinner
  -- strict thread matches above, loose matches on Subject: below --
2011-01-07 11:30 [PATCH 0/8] xfs: prevent corruption due to overlapping AIO DIO V3 Dave Chinner
2011-01-07 11:30 ` [PATCH 4/8] xfs: introduce xfs_rw_lock() helpers for locking the inode Dave Chinner
2011-01-10 19:23   ` Christoph Hellwig
2011-01-10 22:26     ` Dave Chinner
2011-01-04  4:48 [PATCH 0/8] xfs: prevent corruption due to overlapping AIO DIO V2 Dave Chinner
2011-01-04  4:48 ` [PATCH 4/8] xfs: introduce xfs_rw_lock() helpers for locking the inode Dave Chinner
2011-01-05  1:54   ` Alex Elder
2011-01-05  7:55     ` Dave Chinner

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1294781763.3115.9.camel@doink \
    --to=aelder@sgi.com \
    --cc=david@fromorbit.com \
    --cc=hch@infradead.org \
    --cc=xfs@oss.sgi.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox