xfs
[Top] [All Lists]

Re: [PATCH 1/5] xfs: take the ilock around xfs_bmapi_read in xfs_zero_re

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/5] xfs: take the ilock around xfs_bmapi_read in xfs_zero_remaining_bytes
From: Ben Myers <bpm@xxxxxxx>
Date: Thu, 5 Dec 2013 14:37:37 -0600
Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20131205203115.GA29897@dastard>
References: <20131205155830.620826868@xxxxxxxxxxxxxxxxxxxxxx> <20131205155951.199565525@xxxxxxxxxxxxxxxxxxxxxx> <20131205203115.GA29897@dastard>
User-agent: Mutt/1.5.20 (2009-06-14)
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@xxxxxx>
> > 
> > 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.

<Prev in Thread] Current Thread [Next in Thread>