xfs: take the ILOCK when readdir inspects directory mapping data

I was poking around in the directory code while diagnosing online fsck
bugs, and noticed that xfs_readdir doesn't actually take the directory
ILOCK when it calls xfs_dir2_isblock.  xfs_dir_open most probably loaded
the data fork mappings and the VFS took i_rwsem (aka IOLOCK_SHARED) so
we're protected against writer threads, but we really need to follow the
locking model like we do in other places.

To avoid unnecessarily cycling the ILOCK for fairly small directories,
change the block/leaf _getdents functions to consume the ILOCK hold that
the parent readdir function took to decide on a _getdents implementation.

It is ok to cycle the ILOCK in readdir because the VFS takes the IOLOCK
in the appropriate mode during lookups and writes, and we don't want to
be holding the ILOCK when we copy directory entries to userspace in case
there's a page fault.  We really only need it to protect against data
fork lookups, like we do for other files.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Dave Chinner <dchinner@redhat.com>

diff --git a/fs/xfs/xfs_dir2_readdir.c b/fs/xfs/xfs_dir2_readdir.c
index 8310005af00f20997fd8ca0a51b2e5843b866677..a7174a5b3203b321d3c2ab2d1e1636884ee90154 100644 (file)
--- a/fs/xfs/xfs_dir2_readdir.c
+++ b/fs/xfs/xfs_dir2_readdir.c
@@ -138,7 +138,8 @@ xfs_dir2_sf_getdents(
 STATIC int
 xfs_dir2_block_getdents(
        struct xfs_da_args      *args,
-       struct dir_context      *ctx)
+       struct dir_context      *ctx,
+       unsigned int            *lock_mode)
 {
        struct xfs_inode        *dp = args->dp; /* incore directory inode */
        struct xfs_buf          *bp;            /* buffer for block */
@@ -146,7 +147,6 @@ xfs_dir2_block_getdents(
        int                     wantoff;        /* starting block offset */
        xfs_off_t               cook;
        struct xfs_da_geometry  *geo = args->geo;
-       int                     lock_mode;
        unsigned int            offset, next_offset;
        unsigned int            end;
 
@@ -156,12 +156,13 @@ xfs_dir2_block_getdents(
        if (xfs_dir2_dataptr_to_db(geo, ctx->pos) > geo->datablk)
                return 0;
 
-       lock_mode = xfs_ilock_data_map_shared(dp);
        error = xfs_dir3_block_read(args->trans, dp, &bp);
-       xfs_iunlock(dp, lock_mode);
        if (error)
                return error;
 
+       xfs_iunlock(dp, *lock_mode);
+       *lock_mode = 0;
+
        /*
         * Extract the byte offset we start at from the seek pointer.
         * We'll skip entries before this.
@@ -344,7 +345,8 @@ STATIC int
 xfs_dir2_leaf_getdents(
        struct xfs_da_args      *args,
        struct dir_context      *ctx,
-       size_t                  bufsize)
+       size_t                  bufsize,
+       unsigned int            *lock_mode)
 {
        struct xfs_inode        *dp = args->dp;
        struct xfs_mount        *mp = dp->i_mount;
@@ -356,7 +358,6 @@ xfs_dir2_leaf_getdents(
        xfs_dir2_off_t          curoff;         /* current overall offset */
        int                     length;         /* temporary length value */
        int                     byteoff;        /* offset in current block */
-       int                     lock_mode;
        unsigned int            offset = 0;
        int                     error = 0;      /* error return value */
 
@@ -390,13 +391,16 @@ xfs_dir2_leaf_getdents(
                                bp = NULL;
                        }
 
-                       lock_mode = xfs_ilock_data_map_shared(dp);
+                       if (*lock_mode == 0)
+                               *lock_mode = xfs_ilock_data_map_shared(dp);
                        error = xfs_dir2_leaf_readbuf(args, bufsize, &curoff,
                                        &rablk, &bp);
-                       xfs_iunlock(dp, lock_mode);
                        if (error || !bp)
                                break;
 
+                       xfs_iunlock(dp, *lock_mode);
+                       *lock_mode = 0;
+
                        xfs_dir3_data_check(dp, bp);
                        /*
                         * Find our position in the block.
@@ -496,7 +500,7 @@ xfs_dir2_leaf_getdents(
  *
  * If supplied, the transaction collects locked dir buffers to avoid
  * nested buffer deadlocks.  This function does not dirty the
- * transaction.  The caller should ensure that the inode is locked
+ * transaction.  The caller must hold the IOLOCK (shared or exclusive)
  * before calling this function.
  */
 int
@@ -507,8 +511,9 @@ xfs_readdir(
        size_t                  bufsize)
 {
        struct xfs_da_args      args = { NULL };
-       int                     rval;
-       int                     v;
+       unsigned int            lock_mode;
+       int                     isblock;
+       int                     error;
 
        trace_xfs_readdir(dp);
 
@@ -516,6 +521,7 @@ xfs_readdir(
                return -EIO;
 
        ASSERT(S_ISDIR(VFS_I(dp)->i_mode));
+       ASSERT(xfs_isilocked(dp, XFS_IOLOCK_SHARED | XFS_IOLOCK_EXCL));
        XFS_STATS_INC(dp->i_mount, xs_dir_getdents);
 
        args.dp = dp;
@@ -523,13 +529,22 @@ xfs_readdir(
        args.trans = tp;
 
        if (dp->i_df.if_format == XFS_DINODE_FMT_LOCAL)
-               rval = xfs_dir2_sf_getdents(&args, ctx);
-       else if ((rval = xfs_dir2_isblock(&args, &v)))
-               ;
-       else if (v)
-               rval = xfs_dir2_block_getdents(&args, ctx);
-       else
-               rval = xfs_dir2_leaf_getdents(&args, ctx, bufsize);
+               return xfs_dir2_sf_getdents(&args, ctx);
 
-       return rval;
+       lock_mode = xfs_ilock_data_map_shared(dp);
+       error = xfs_dir2_isblock(&args, &isblock);
+       if (error)
+               goto out_unlock;
+
+       if (isblock) {
+               error = xfs_dir2_block_getdents(&args, ctx, &lock_mode);
+               goto out_unlock;
+       }
+
+       error = xfs_dir2_leaf_getdents(&args, ctx, bufsize, &lock_mode);
+
+out_unlock:
+       if (lock_mode)
+               xfs_iunlock(dp, lock_mode);
+       return error;
 }