| Message ID | 20181203160652.25078-3-josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Delayed iput fixes | expand |
On 3.12.18 г. 18:06 ч., Josef Bacik wrote: > The cleaner thread usually takes care of delayed iputs, with the > exception of the btrfs_end_transaction_throttle path. The cleaner > thread only gets woken up every 30 seconds, so instead wake it up to do > it's work so that we can free up that space as quickly as possible. This description misses any rationale whatsoever about why the cleaner needs to be woken up more frequently than 30 seconds (and IMO this is the most important question that needs answering). Also have you done any measurements of the number of processed delayed inodes with this change. Given the behavior you so desire why not just make delayed iputs to be run via schedule_work on the global workqueue and be done with it? I'm sure the latency will be better than the current 30 seconds one :) > > Reviewed-by: Filipe Manana <fdmanana@suse.com> > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > --- > fs/btrfs/ctree.h | 3 +++ > fs/btrfs/disk-io.c | 3 +++ > fs/btrfs/inode.c | 2 ++ > 3 files changed, 8 insertions(+) > > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > index c8ddbacb6748..dc56a4d940c3 100644 > --- a/fs/btrfs/ctree.h > +++ b/fs/btrfs/ctree.h > @@ -769,6 +769,9 @@ bool btrfs_pinned_by_swapfile(struct btrfs_fs_info *fs_info, void *ptr); > */ > #define BTRFS_FS_BALANCE_RUNNING 18 > > +/* Indicate that the cleaner thread is awake and doing something. */ > +#define BTRFS_FS_CLEANER_RUNNING 19 > + > struct btrfs_fs_info { > u8 fsid[BTRFS_FSID_SIZE]; > u8 chunk_tree_uuid[BTRFS_UUID_SIZE]; > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index c5918ff8241b..f40f6fdc1019 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -1669,6 +1669,8 @@ static int cleaner_kthread(void *arg) > while (1) { > again = 0; > > + set_bit(BTRFS_FS_CLEANER_RUNNING, &fs_info->flags); > + > /* Make the cleaner go to sleep early. */ > if (btrfs_need_cleaner_sleep(fs_info)) > goto sleep; > @@ -1715,6 +1717,7 @@ static int cleaner_kthread(void *arg) > */ > btrfs_delete_unused_bgs(fs_info); > sleep: > + clear_bit(BTRFS_FS_CLEANER_RUNNING, &fs_info->flags); > if (kthread_should_park()) > kthread_parkme(); > if (kthread_should_stop()) > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 8ac7abe2ae9b..0b9f3e482cea 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -3264,6 +3264,8 @@ void btrfs_add_delayed_iput(struct inode *inode) > ASSERT(list_empty(&binode->delayed_iput)); > list_add_tail(&binode->delayed_iput, &fs_info->delayed_iputs); > spin_unlock(&fs_info->delayed_iput_lock); > + if (!test_bit(BTRFS_FS_CLEANER_RUNNING, &fs_info->flags)) > + wake_up_process(fs_info->cleaner_kthread); > } > > void btrfs_run_delayed_iputs(struct btrfs_fs_info *fs_info) >
On Tue, Dec 04, 2018 at 11:21:14AM +0200, Nikolay Borisov wrote: > > > On 3.12.18 г. 18:06 ч., Josef Bacik wrote: > > The cleaner thread usually takes care of delayed iputs, with the > > exception of the btrfs_end_transaction_throttle path. The cleaner > > thread only gets woken up every 30 seconds, so instead wake it up to do > > it's work so that we can free up that space as quickly as possible. > > This description misses any rationale whatsoever about why the cleaner > needs to be woken up more frequently than 30 seconds (and IMO this is > the most important question that needs answering). > Yeah I'll add that. > Also have you done any measurements of the number of processed delayed > inodes with this change. Given the behavior you so desire why not just > make delayed iputs to be run via schedule_work on the global workqueue > and be done with it? I'm sure the latency will be better than the > current 30 seconds one :) We already have the cleaner thread to do this work, and it sets up for the snapshot drop stuff to be run as well. We could probably add another delayed work thing, but I would rather do that in a different patch. Thanks, Josef
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index c8ddbacb6748..dc56a4d940c3 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -769,6 +769,9 @@ bool btrfs_pinned_by_swapfile(struct btrfs_fs_info *fs_info, void *ptr); */ #define BTRFS_FS_BALANCE_RUNNING 18 +/* Indicate that the cleaner thread is awake and doing something. */ +#define BTRFS_FS_CLEANER_RUNNING 19 + struct btrfs_fs_info { u8 fsid[BTRFS_FSID_SIZE]; u8 chunk_tree_uuid[BTRFS_UUID_SIZE]; diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index c5918ff8241b..f40f6fdc1019 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -1669,6 +1669,8 @@ static int cleaner_kthread(void *arg) while (1) { again = 0; + set_bit(BTRFS_FS_CLEANER_RUNNING, &fs_info->flags); + /* Make the cleaner go to sleep early. */ if (btrfs_need_cleaner_sleep(fs_info)) goto sleep; @@ -1715,6 +1717,7 @@ static int cleaner_kthread(void *arg) */ btrfs_delete_unused_bgs(fs_info); sleep: + clear_bit(BTRFS_FS_CLEANER_RUNNING, &fs_info->flags); if (kthread_should_park()) kthread_parkme(); if (kthread_should_stop()) diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 8ac7abe2ae9b..0b9f3e482cea 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -3264,6 +3264,8 @@ void btrfs_add_delayed_iput(struct inode *inode) ASSERT(list_empty(&binode->delayed_iput)); list_add_tail(&binode->delayed_iput, &fs_info->delayed_iputs); spin_unlock(&fs_info->delayed_iput_lock); + if (!test_bit(BTRFS_FS_CLEANER_RUNNING, &fs_info->flags)) + wake_up_process(fs_info->cleaner_kthread); } void btrfs_run_delayed_iputs(struct btrfs_fs_info *fs_info)