On Sat 22-08-15 09:11:54, Dave Chinner wrote:
> On Fri, Aug 21, 2015 at 07:55:22PM +0200, Jan Kara wrote:
> > Users have occasionally reported that file type for some directory
> > entries is wrong. This mostly happened after updating libraries some
> > libraries. After some debugging the problem was traced down to
> > xfs_dir2_node_replace(). The function uses args->filetype as a file type
> > to store in the replaced directory entry however it also calls
> > xfs_da3_node_lookup_int() which will store file type of the current
> > directory entry in args->filetype. Thus we fail to change file type of a
> > directory entry to a proper type.
> >
> > Fix the problem by storing new file type in a local variable before
> > calling xfs_da3_node_lookup_int().
> >
> > Reported-by: Giacomo Comes <comes@xxxxxxxx>
> > Signed-off-by: Jan Kara <jack@xxxxxxxx>
>
> So this is being triggered by a rename() operation on a large
> directory? node format is the optimised form form for large dirs, so
> I suspect that's why only few people see this. Can you see if you
> can write a reproducer for it baseed on a large single directory and
> renaming two files of different types (e.g. BLKDEV over REG) to see
> if the cause is that simple?
Yes, I've tried and for a large enough directory renaming symlink over a
regular file is all that is needed to corrupt the file type in the
directory. Should I write a dedicated test for this or is there something
that already excercises directory code? I know about fsstress runs but
those would be hard to tweak to trigger this I guess.
Honza
> > ---
> > fs/xfs/libxfs/xfs_dir2_node.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_dir2_node.c b/fs/xfs/libxfs/xfs_dir2_node.c
> > index 41b80d3d3877..1006710a7c92 100644
> > --- a/fs/xfs/libxfs/xfs_dir2_node.c
> > +++ b/fs/xfs/libxfs/xfs_dir2_node.c
> > @@ -2132,6 +2132,7 @@ xfs_dir2_node_replace(
> > int error; /* error return value */
> > int i; /* btree level */
> > xfs_ino_t inum; /* new inode number */
> > + int ftype; /* new file type */
> > xfs_dir2_leaf_t *leaf; /* leaf structure */
> > xfs_dir2_leaf_entry_t *lep; /* leaf entry being changed */
> > int rval; /* internal return value */
> > @@ -2145,7 +2146,12 @@ xfs_dir2_node_replace(
> > state = xfs_da_state_alloc();
> > state->args = args;
> > state->mp = args->dp->i_mount;
> > + /*
> > + * We have to save new inode number and ftype since
> > + * xfs_da3_node_lookup_int() is going to overwrite them
> > + */
> > inum = args->inumber;
> > + ftype = args->filetype;
> > /*
> > * Lookup the entry to change in the btree.
> > */
> > @@ -2183,7 +2189,7 @@ xfs_dir2_node_replace(
> > * Fill in the new inode number and log the entry.
> > */
> > dep->inumber = cpu_to_be64(inum);
> > - args->dp->d_ops->data_put_ftype(dep, args->filetype);
> > + args->dp->d_ops->data_put_ftype(dep, ftype);
> > xfs_dir2_data_log_entry(args, state->extrablk.bp, dep);
> > rval = 0;
> > }
>
> The change looks sane (i'll add whitespace around the comments when
> i commit) and I'll also add the stable cc. I would like to have a
> reproducer to test it, though ;)
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
|