From: Dennis Zhou <dennis@kernel.org>
To: David Sterba <dsterba@suse.cz>
Cc: David Sterba <dsterba@suse.com>, Chris Mason <clm@fb.com>,
Josef Bacik <josef@toxicpanda.com>,
Omar Sandoval <osandov@osandov.com>,
kernel-team@fb.com, linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 05/19] btrfs: add the beginning of async discard, discard workqueue
Date: Tue, 15 Oct 2019 15:57:49 -0400 [thread overview]
Message-ID: <20191015195749.GC82683@dennisz-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <20191015124919.GW2751@twin.jikos.cz>
On Tue, Oct 15, 2019 at 02:49:19PM +0200, David Sterba wrote:
> On Mon, Oct 07, 2019 at 04:17:36PM -0400, Dennis Zhou wrote:
> > --- a/fs/btrfs/block-group.h
> > +++ b/fs/btrfs/block-group.h
> > @@ -115,7 +115,11 @@ struct btrfs_block_group_cache {
> > /* For read-only block groups */
> > struct list_head ro_list;
> >
> > + /* For discard operations */
> > atomic_t trimming;
> > + struct list_head discard_list;
> > + int discard_index;
> > + u64 discard_delay;
> >
> > /* For dirty block groups */
> > struct list_head dirty_list;
> > @@ -157,6 +161,12 @@ struct btrfs_block_group_cache {
> > struct btrfs_full_stripe_locks_tree full_stripe_locks_root;
> > };
> >
> > +static inline
> > +u64 btrfs_block_group_end(struct btrfs_block_group_cache *cache)
> > +{
> > + return (cache->key.objectid + cache->key.offset);
> > +}
> > +
> > #ifdef CONFIG_BTRFS_DEBUG
> > static inline int btrfs_should_fragment_free_space(
> > struct btrfs_block_group_cache *block_group)
> > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > index 1877586576aa..419445868909 100644
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -438,6 +438,17 @@ struct btrfs_full_stripe_locks_tree {
> > struct mutex lock;
> > };
> >
> > +/* discard control */
>
> This is going to be 'fix everywhere too' comment, please start comments
> with capital letter.
I've done a pass at uppercasing everything. I think I've caught most of
them.
>
> > +#define BTRFS_NR_DISCARD_LISTS 1
>
> Constants and defines should be documented
>
Yeah sounds good. I've added comments. A few may not have comments until
later, but I think by the end of the series everything has the right
comments.
> > --- /dev/null
> > +++ b/fs/btrfs/discard.c
> > @@ -0,0 +1,200 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2019 Facebook. All rights reserved.
> > + */
>
> With the SPDX in place and immutable git history, the copyright notices
> are not necessary and we don't add them to new files anymore.
>
Ah okay. I wasn't exactly sure why those got added when they did.
> > +void btrfs_add_to_discard_list(struct btrfs_discard_ctl *discard_ctl,
> > + struct btrfs_block_group_cache *cache)
> > +{
> > + u64 now = ktime_get_ns();
>
> Variable used only once, can be removed and ktime_get_ns called
> directly.
>
Done.
> > + spin_lock(&discard_ctl->lock);
> > +
> > + if (list_empty(&cache->discard_list))
> > + cache->discard_delay = now + BTRFS_DISCARD_DELAY;
>
> ->discard_delay does not seem to be a delay but an expiration time, so
> this is a bit confusing. BTRFS_DISCARD_DELAY is the delay time, that's
> clear.
>
I added a comment explaining the premise. I just didn't want a block
group to start discarding immediately if we just created it, so give
some chance for the lba to be reused. So, discard_delay holds the
time to begin discarding. I'll try and figure out a better name.
Maybe discard_eligible_time (idk it seems long)?
> > + list_move_tail(&cache->discard_list,
> > + btrfs_get_discard_list(discard_ctl, cache));
> > +
> > + spin_unlock(&discard_ctl->lock);
> > +}
>
> > --- /dev/null
> > +++ b/fs/btrfs/discard.h
> > @@ -0,0 +1,49 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2019 Facebook. All rights reserved.
> > + */
> > +
> > +#ifndef BTRFS_DISCARD_H
> > +#define BTRFS_DISCARD_H
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/workqueue.h>
> > +
> > +#include "ctree.h"
>
> Is it possible to avoid including ctree.h here? Like adding forward
> declarations and defining the helpers in .c (that will have to include
> ctree.h anyway). The includes have become very cluttered and untangling
> the dependencies is ongoing work so it would be good to avoid adding
> extra work.
>
I moved the two inline's to the .c file and then just struct
declarations was enough.
> > +void btrfs_add_to_discard_list(struct btrfs_discard_ctl *discard_ctl,
> > + struct btrfs_block_group_cache *cache);
> > +
> > +void btrfs_discard_cancel_work(struct btrfs_discard_ctl *discard_ctl,
> > + struct btrfs_block_group_cache *cache);
> > +void btrfs_discard_schedule_work(struct btrfs_discard_ctl *discard_ctl,
> > + bool override);
> > +void btrfs_discard_resume(struct btrfs_fs_info *fs_info);
> > +void btrfs_discard_stop(struct btrfs_fs_info *fs_info);
> > +void btrfs_discard_init(struct btrfs_fs_info *fs_info);
> > +void btrfs_discard_cleanup(struct btrfs_fs_info *fs_info);
> > +
> > +static inline
> > +bool btrfs_run_discard_work(struct btrfs_discard_ctl *discard_ctl)
> > +{
> > + struct btrfs_fs_info *fs_info = container_of(discard_ctl,
> > + struct btrfs_fs_info,
> > + discard_ctl);
> > +
> > + return (!(fs_info->sb->s_flags & SB_RDONLY) &&
> > + test_bit(BTRFS_FS_DISCARD_RUNNING, &fs_info->flags));
> > +}
> > +
> > +static inline
> > +void btrfs_discard_queue_work(struct btrfs_discard_ctl *discard_ctl,
> > + struct btrfs_block_group_cache *cache)
> > +{
> > + if (!cache || !btrfs_test_opt(cache->fs_info, DISCARD_ASYNC))
> > + return;
> > +
> > + btrfs_add_to_discard_list(discard_ctl, cache);
> > + if (!delayed_work_pending(&discard_ctl->work))
> > + btrfs_discard_schedule_work(discard_ctl, false);
> > +}
>
> These two would need full fs_info definition but they don't seem to be
> called in performance sensitive code so a full function call is ok here.
>
Done.
> > --- a/fs/btrfs/super.c
> > +++ b/fs/btrfs/super.c
> > @@ -313,6 +316,7 @@ enum {
> > Opt_datasum, Opt_nodatasum,
> > Opt_defrag, Opt_nodefrag,
> > Opt_discard, Opt_nodiscard,
> > + Opt_discard_version,
>
> This is probably copied from space_cache options, 'version' does not
> fit discard, it could be 'mode'
>
So the initial version I actually named them discard v1 and v2, Omar
mentioned I should probably give them separate flags and why it's now
sync and async. I renamed it to mode.
> > Opt_nologreplay,
> > Opt_norecovery,
> > Opt_ratio,
> > case Opt_space_cache_version:
Thanks,
Dennis
next prev parent reply other threads:[~2019-10-15 19:57 UTC|newest]
Thread overview: 71+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-07 20:17 [RFC PATCH 00/19] btrfs: async discard support Dennis Zhou
2019-10-07 20:17 ` [PATCH 01/19] bitmap: genericize percpu bitmap region iterators Dennis Zhou
2019-10-07 20:26 ` Josef Bacik
2019-10-07 22:24 ` Dennis Zhou
2019-10-15 12:11 ` David Sterba
2019-10-15 18:35 ` Dennis Zhou
2019-10-07 20:17 ` [PATCH 02/19] btrfs: rename DISCARD opt to DISCARD_SYNC Dennis Zhou
2019-10-07 20:27 ` Josef Bacik
2019-10-08 11:12 ` Johannes Thumshirn
2019-10-11 9:19 ` Nikolay Borisov
2019-10-07 20:17 ` [PATCH 03/19] btrfs: keep track of which extents have been discarded Dennis Zhou
2019-10-07 20:37 ` Josef Bacik
2019-10-07 22:38 ` Dennis Zhou
2019-10-10 13:40 ` Josef Bacik
2019-10-11 16:15 ` Dennis Zhou
2019-10-08 12:46 ` Nikolay Borisov
2019-10-11 16:08 ` Dennis Zhou
2019-10-15 12:17 ` David Sterba
2019-10-15 19:58 ` Dennis Zhou
2019-10-07 20:17 ` [PATCH 04/19] btrfs: keep track of cleanliness of the bitmap Dennis Zhou
2019-10-10 14:16 ` Josef Bacik
2019-10-11 16:17 ` Dennis Zhou
2019-10-15 12:23 ` David Sterba
2019-10-07 20:17 ` [PATCH 05/19] btrfs: add the beginning of async discard, discard workqueue Dennis Zhou
2019-10-10 14:38 ` Josef Bacik
2019-10-15 12:49 ` David Sterba
2019-10-15 19:57 ` Dennis Zhou [this message]
2019-10-07 20:17 ` [PATCH 06/19] btrfs: handle empty block_group removal Dennis Zhou
2019-10-10 15:00 ` Josef Bacik
2019-10-11 16:52 ` Dennis Zhou
2019-10-07 20:17 ` [PATCH 07/19] btrfs: discard one region at a time in async discard Dennis Zhou
2019-10-10 15:22 ` Josef Bacik
2019-10-14 19:42 ` Dennis Zhou
2019-10-07 20:17 ` [PATCH 08/19] btrfs: track discardable extents for asnyc discard Dennis Zhou
2019-10-10 15:36 ` Josef Bacik
2019-10-14 19:50 ` Dennis Zhou
2019-10-15 13:12 ` David Sterba
2019-10-15 18:41 ` Dennis Zhou
2019-10-07 20:17 ` [PATCH 09/19] btrfs: keep track of discardable_bytes Dennis Zhou
2019-10-10 15:38 ` Josef Bacik
2019-10-07 20:17 ` [PATCH 10/19] btrfs: calculate discard delay based on number of extents Dennis Zhou
2019-10-10 15:41 ` Josef Bacik
2019-10-11 18:07 ` Dennis Zhou
2019-10-07 20:17 ` [PATCH 11/19] btrfs: add bps discard rate limit Dennis Zhou
2019-10-10 15:47 ` Josef Bacik
2019-10-14 19:56 ` Dennis Zhou
2019-10-07 20:17 ` [PATCH 12/19] btrfs: limit max discard size for async discard Dennis Zhou
2019-10-10 16:16 ` Josef Bacik
2019-10-14 19:57 ` Dennis Zhou
2019-10-07 20:17 ` [PATCH 13/19] btrfs: have multiple discard lists Dennis Zhou
2019-10-10 16:51 ` Josef Bacik
2019-10-14 20:04 ` Dennis Zhou
2019-10-07 20:17 ` [PATCH 14/19] btrfs: only keep track of data extents for async discard Dennis Zhou
2019-10-10 16:53 ` Josef Bacik
2019-10-07 20:17 ` [PATCH 15/19] btrfs: load block_groups into discard_list on mount Dennis Zhou
2019-10-10 17:11 ` Josef Bacik
2019-10-14 20:17 ` Dennis Zhou
2019-10-14 23:38 ` David Sterba
2019-10-15 15:42 ` Dennis Zhou
2019-10-07 20:17 ` [PATCH 16/19] btrfs: keep track of discard reuse stats Dennis Zhou
2019-10-10 17:13 ` Josef Bacik
2019-10-07 20:17 ` [PATCH 17/19] btrfs: add async discard header Dennis Zhou
2019-10-10 17:13 ` Josef Bacik
2019-10-07 20:17 ` [PATCH 18/19] btrfs: increase the metadata allowance for the free_space_cache Dennis Zhou
2019-10-10 17:16 ` Josef Bacik
2019-10-07 20:17 ` [PATCH 19/19] btrfs: make smaller extents more likely to go into bitmaps Dennis Zhou
2019-10-10 17:17 ` Josef Bacik
2019-10-11 7:49 ` [RFC PATCH 00/19] btrfs: async discard support Nikolay Borisov
2019-10-14 21:05 ` Dennis Zhou
2019-10-15 12:08 ` David Sterba
2019-10-15 15:41 ` Dennis Zhou
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191015195749.GC82683@dennisz-mbp.dhcp.thefacebook.com \
--to=dennis@kernel.org \
--cc=clm@fb.com \
--cc=dsterba@suse.com \
--cc=dsterba@suse.cz \
--cc=josef@toxicpanda.com \
--cc=kernel-team@fb.com \
--cc=linux-btrfs@vger.kernel.org \
--cc=osandov@osandov.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).