From: Ben Myers <bpm@sgi.com>
To: Dave Chinner <david@fromorbit.com>
Cc: Christoph Hellwig <hch@infradead.org>, xfs@oss.sgi.com
Subject: Re: [PATCH 1/5] xfs: take the ilock around xfs_bmapi_read in xfs_zero_remaining_bytes
Date: Thu, 5 Dec 2013 14:37:37 -0600 [thread overview]
Message-ID: <20131205203737.GM1935@sgi.com> (raw)
In-Reply-To: <20131205203115.GA29897@dastard>
On Fri, Dec 06, 2013 at 07:31:15AM +1100, Dave Chinner wrote:
> On Thu, Dec 05, 2013 at 07:58:31AM -0800, Christoph Hellwig wrote:
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> >
> > Index: xfs/fs/xfs/xfs_bmap_util.c
> > ===================================================================
> > --- xfs.orig/fs/xfs/xfs_bmap_util.c 2013-12-05 11:37:57.791685284 +0100
> > +++ xfs/fs/xfs/xfs_bmap_util.c 2013-12-05 11:39:43.599683113 +0100
> > @@ -1147,6 +1147,7 @@ xfs_zero_remaining_bytes(
> > xfs_mount_t *mp = ip->i_mount;
> > int nimap;
> > int error = 0;
> > + uint lock_mode;
> >
> > /*
> > * Avoid doing I/O beyond eof - it's not necessary
> > @@ -1159,11 +1160,15 @@ xfs_zero_remaining_bytes(
> > if (endoff > XFS_ISIZE(ip))
> > endoff = XFS_ISIZE(ip);
> >
> > + lock_mode = xfs_ilock_map_shared(ip);
> > +
> > bp = xfs_buf_get_uncached(XFS_IS_REALTIME_INODE(ip) ?
> > mp->m_rtdev_targp : mp->m_ddev_targp,
> > BTOBB(mp->m_sb.sb_blocksize), 0);
>
> This now holds the ilock over data IO, which is not allowed to be
> done as data IO completion can require the ilock to be taken. Yes,
> the code specifically avoids all these problems, but the general
> rule is that ilock is only held over metadata IO operations, not
> data IO. If we need data IO serialisation, then we use the iolock.
>
> So, while this protects the extent tree, it also violates other
> long-standing conventions for locking. Given that the code is
> special, I'mnot opposed to making a special rule for this one
> function, but it needs to be commented as to why this is a valid
> thing to do in this function....
Maybe it would be better if the ilock could be taken and dropped within the
loop.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
next prev parent reply other threads:[~2013-12-05 20:37 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-12-05 15:58 [PATCH 0/5] extent list locking fixes Christoph Hellwig
2013-12-05 15:58 ` [PATCH 1/5] xfs: take the ilock around xfs_bmapi_read in xfs_zero_remaining_bytes Christoph Hellwig
2013-12-05 19:38 ` Ben Myers
2013-12-05 20:31 ` Dave Chinner
2013-12-05 20:37 ` Ben Myers [this message]
2013-12-05 20:40 ` Christoph Hellwig
2013-12-05 15:58 ` [PATCH 2/5] xfs: use xfs_ilock_map_shared in xfs_qm_dqtobp Christoph Hellwig
2013-12-05 19:46 ` Ben Myers
2013-12-05 20:41 ` Dave Chinner
2013-12-05 20:53 ` Christoph Hellwig
2013-12-05 21:03 ` Dave Chinner
2013-12-05 15:58 ` [PATCH 3/5] xfs: use xfs_ilock_map_shared in xfs_qm_dqiterate Christoph Hellwig
2013-12-05 19:48 ` Ben Myers
2013-12-05 20:45 ` Dave Chinner
2013-12-05 15:58 ` [PATCH 4/5] xfs: use xfs_ilock_map_shared in xfs_attr_get Christoph Hellwig
2013-12-05 19:57 ` Ben Myers
2013-12-05 20:59 ` Dave Chinner
2013-12-05 21:01 ` Christoph Hellwig
2013-12-05 21:05 ` Dave Chinner
2013-12-05 21:10 ` Christoph Hellwig
2013-12-05 21:17 ` Dave Chinner
2013-12-05 15:58 ` [PATCH 5/5] xfs: assert that we hold the ilock for extent map access Christoph Hellwig
2013-12-05 20:11 ` Ben Myers
2013-12-05 21:10 ` Dave Chinner
2013-12-05 21:22 ` Christoph Hellwig
2013-12-05 21:40 ` 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=20131205203737.GM1935@sgi.com \
--to=bpm@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