Re: [PATCH v3 5/5] xfs: Switch to iomap for SEEK_HOLE / SEEK_DATA

[Dave Chinner <david@fromorbit.com>]

On Mon, Jun 26, 2017 at 05:34:35PM -0700, Darrick J. Wong wrote:
> [adding Christoph to cc]
> 
> On Mon, Jun 26, 2017 at 04:25:18PM +0200, Andreas Gruenbacher wrote:
> > Switch to the iomap_seek_hole_data vfs helper for implementing lseek
> > SEEK_HOLE / SEEK_DATA.  __xfs_seek_hole_data can go away once it's no
> > longer used by the quota code.
> > 
> > Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
> > ---
> >  fs/xfs/xfs_file.c | 21 +++++----------------
> >  1 file changed, 5 insertions(+), 16 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > index 962dafd..94fe89a 100644
> > --- a/fs/xfs/xfs_file.c
> > +++ b/fs/xfs/xfs_file.c
> > @@ -1131,29 +1131,18 @@ xfs_seek_hole_data(
> >  	struct xfs_inode	*ip = XFS_I(inode);
> >  	struct xfs_mount	*mp = ip->i_mount;
> >  	uint			lock;
> > -	loff_t			offset, end;
> > -	int			error = 0;
> > +	loff_t			offset;
> >  
> >  	if (XFS_FORCED_SHUTDOWN(mp))
> >  		return -EIO;
> >  
> >  	lock = xfs_ilock_data_map_shared(ip);
> > -
> > -	end = i_size_read(inode);
> > -	offset = __xfs_seek_hole_data(inode, start, end, whence);
> > -	if (offset < 0) {
> > -		error = offset;
> > -		goto out_unlock;
> > -	}
> > -
> > -	offset = vfs_setpos(file, offset, inode->i_sb->s_maxbytes);
> > -
> > -out_unlock:
> > +	offset = iomap_seek_hole_data(inode, start, whence, &xfs_iomap_ops);
> 
> Hm.  We grab the data map ilock above, then we call
> iomap_seek_hole_data, which (eventually) calls xfs_file_iomap_begin,
> which tries to grab the data map ilock.  We shouldn't be grabbing the
> ilock twice, obviously, but on the other hand...
> 
> ...under the old code, we'd take the ilock and do the whole block map
> and page cache scans without ever dropping the ilock.

Which I'm pretty sure I've previously pointed out is broken and
needed fixing (lockdep reports, IIRC), as the lock order is iolock
-> page lock -> ilock.

(yes, I'm using "iolock" as shorthand for inode->i_rwsem)

> This new iomap
> based thing only holds the ilock during ->iomap_begin, which makes me
> worry that someone else can wander in and mess with things while we're
> looking for holes/data?

Locking won't prevent seek races with concurrent modifications from
the perspective of userspace.

i.e. we can lock the inode down, seek to data, unlock it, and before
we get back to userspace something else punches out that data. So by
the time the app gets to use the position set by the seek, there's a
hole where it's being told there *was* data....

-Dave.
-- 
Dave Chinner
david@fromorbit.com