On Fri, Sep 28, 2018 at 12:19 PM Josef Bacik <jo...@toxicpanda.com> wrote: > > We're getting a lockdep splat because we take the dio_sem under the > log_mutex. What we really need is to protect fsync() from logging an > extent map for an extent we never waited on higher up, so just guard the > whole thing with dio_sem. > > Signed-off-by: Josef Bacik <jo...@toxicpanda.com> Reviewed-by: Filipe Manana <fdman...@suse.com>
Looks good, thanks. However as David said, it would be nice to have a sample trace pasted in the changelog (several fstests test cases trigger this often). > --- > fs/btrfs/file.c | 12 ++++++++++++ > fs/btrfs/tree-log.c | 2 -- > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c > index 095f0bb86bb7..c07110edb9de 100644 > --- a/fs/btrfs/file.c > +++ b/fs/btrfs/file.c > @@ -2079,6 +2079,14 @@ int btrfs_sync_file(struct file *file, loff_t start, > loff_t end, int datasync) > goto out; > > inode_lock(inode); > + > + /* > + * We take the dio_sem here because the tree log stuff can race with > + * lockless dio writes and get an extent map logged for an extent we > + * never waited on. We need it this high up for lockdep reasons. > + */ > + down_write(&BTRFS_I(inode)->dio_sem); > + > atomic_inc(&root->log_batch); > > /* > @@ -2087,6 +2095,7 @@ int btrfs_sync_file(struct file *file, loff_t start, > loff_t end, int datasync) > */ > ret = btrfs_wait_ordered_range(inode, start, len); > if (ret) { > + up_write(&BTRFS_I(inode)->dio_sem); > inode_unlock(inode); > goto out; > } > @@ -2110,6 +2119,7 @@ int btrfs_sync_file(struct file *file, loff_t start, > loff_t end, int datasync) > * checked called fsync. > */ > ret = filemap_check_wb_err(inode->i_mapping, file->f_wb_err); > + up_write(&BTRFS_I(inode)->dio_sem); > inode_unlock(inode); > goto out; > } > @@ -2128,6 +2138,7 @@ int btrfs_sync_file(struct file *file, loff_t start, > loff_t end, int datasync) > trans = btrfs_start_transaction(root, 0); > if (IS_ERR(trans)) { > ret = PTR_ERR(trans); > + up_write(&BTRFS_I(inode)->dio_sem); > inode_unlock(inode); > goto out; > } > @@ -2149,6 +2160,7 @@ int btrfs_sync_file(struct file *file, loff_t start, > loff_t end, int datasync) > * file again, but that will end up using the synchronization > * inside btrfs_sync_log to keep things safe. > */ > + up_write(&BTRFS_I(inode)->dio_sem); > inode_unlock(inode); > > /* > diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c > index 1650dc44a5e3..66b7e059b765 100644 > --- a/fs/btrfs/tree-log.c > +++ b/fs/btrfs/tree-log.c > @@ -4374,7 +4374,6 @@ static int btrfs_log_changed_extents(struct > btrfs_trans_handle *trans, > > INIT_LIST_HEAD(&extents); > > - down_write(&inode->dio_sem); > write_lock(&tree->lock); > test_gen = root->fs_info->last_trans_committed; > logged_start = start; > @@ -4440,7 +4439,6 @@ static int btrfs_log_changed_extents(struct > btrfs_trans_handle *trans, > } > WARN_ON(!list_empty(&extents)); > write_unlock(&tree->lock); > - up_write(&inode->dio_sem); > > btrfs_release_path(path); > if (!ret) > -- > 2.14.3 > -- Filipe David Manana, “Whether you think you can, or you think you can't — you're right.”