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 _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
