| Message ID | 20190515150247.24776-1-fdmanana@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] Btrfs: fix fsync not persisting changed attributes of a directory | expand |
On 15.05.19 г. 18:02 ч., fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > When replaying a log that contains a new file or directory name that needs > to be added to its parent directory, we end up updating the mtime and the > ctime of the parent directory to the current time after we have set their > values to the correct ones (set at fsync time), efectivelly losing them. > > Sample reproducer: > > $ mkfs.btrfs -f /dev/sdb > $ mount /dev/sdb /mnt > > $ mkdir /mnt/dir > $ touch /mnt/dir/file > > # fsync of the directory is optional, not needed > $ xfs_io -c fsync /mnt/dir > $ xfs_io -c fsync /mnt/dir/file > > $ stat -c %Y /mnt/dir > 1557856079 > > <power failure> > > $ sleep 3 > $ mount /dev/sdb /mnt > $ stat -c %Y /mnt/dir > 1557856082 > > --> should have been 1557856079, the mtime is updated to the current > time when replaying the log > > Fix this by not updating the mtime and ctime to the current time at > btrfs_add_link() when we are replaying a log tree. > > This could be triggered by my recent fsync fuzz tester for fstests, for > which an fstests patch exists titled "fstests: generic, fsync fuzz tester > with fsstress". > > Fixes: e02119d5a7b43 ("Btrfs: Add a write ahead tree log to optimize synchronous operations") > Signed-off-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: Nikolay Borisov <nborisov@suse.com> > --- > fs/btrfs/inode.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index cacde7040329..8e6d7b0bd988 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -6381,8 +6381,18 @@ int btrfs_add_link(struct btrfs_trans_handle *trans, > btrfs_i_size_write(parent_inode, parent_inode->vfs_inode.i_size + > name_len * 2); > inode_inc_iversion(&parent_inode->vfs_inode); > - parent_inode->vfs_inode.i_mtime = parent_inode->vfs_inode.i_ctime = > - current_time(&parent_inode->vfs_inode); > + /* > + * If we are replaying a log tree, we do not want to update the mtime > + * and ctime of the parent directory with the current time, since the > + * log replay procedure is responsible for setting them to their correct > + * values (the ones it had when the fsync was done). > + */ > + if (!test_bit(BTRFS_FS_LOG_RECOVERING, &root->fs_info->flags)) { > + struct timespec64 now = current_time(&parent_inode->vfs_inode); > + > + parent_inode->vfs_inode.i_mtime = now; > + parent_inode->vfs_inode.i_ctime = now; > + } > ret = btrfs_update_inode(trans, root, &parent_inode->vfs_inode); > if (ret) > btrfs_abort_transaction(trans, ret); >
On Wed, May 15, 2019 at 04:02:47PM +0100, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > When replaying a log that contains a new file or directory name that needs > to be added to its parent directory, we end up updating the mtime and the > ctime of the parent directory to the current time after we have set their > values to the correct ones (set at fsync time), efectivelly losing them. > > Sample reproducer: > > $ mkfs.btrfs -f /dev/sdb > $ mount /dev/sdb /mnt > > $ mkdir /mnt/dir > $ touch /mnt/dir/file > > # fsync of the directory is optional, not needed > $ xfs_io -c fsync /mnt/dir > $ xfs_io -c fsync /mnt/dir/file > > $ stat -c %Y /mnt/dir > 1557856079 > > <power failure> > > $ sleep 3 > $ mount /dev/sdb /mnt > $ stat -c %Y /mnt/dir > 1557856082 > > --> should have been 1557856079, the mtime is updated to the current > time when replaying the log > > Fix this by not updating the mtime and ctime to the current time at > btrfs_add_link() when we are replaying a log tree. > > This could be triggered by my recent fsync fuzz tester for fstests, for > which an fstests patch exists titled "fstests: generic, fsync fuzz tester > with fsstress". > > Fixes: e02119d5a7b43 ("Btrfs: Add a write ahead tree log to optimize synchronous operations") > Signed-off-by: Filipe Manana <fdmanana@suse.com> Added to 5.2-rc queue, thanks.
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index cacde7040329..8e6d7b0bd988 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -6381,8 +6381,18 @@ int btrfs_add_link(struct btrfs_trans_handle *trans, btrfs_i_size_write(parent_inode, parent_inode->vfs_inode.i_size + name_len * 2); inode_inc_iversion(&parent_inode->vfs_inode); - parent_inode->vfs_inode.i_mtime = parent_inode->vfs_inode.i_ctime = - current_time(&parent_inode->vfs_inode); + /* + * If we are replaying a log tree, we do not want to update the mtime + * and ctime of the parent directory with the current time, since the + * log replay procedure is responsible for setting them to their correct + * values (the ones it had when the fsync was done). + */ + if (!test_bit(BTRFS_FS_LOG_RECOVERING, &root->fs_info->flags)) { + struct timespec64 now = current_time(&parent_inode->vfs_inode); + + parent_inode->vfs_inode.i_mtime = now; + parent_inode->vfs_inode.i_ctime = now; + } ret = btrfs_update_inode(trans, root, &parent_inode->vfs_inode); if (ret) btrfs_abort_transaction(trans, ret);