Re: [PATCH 05/19] btrfs: add the beginning of async discard, discard workqueue

[Dennis Zhou <dennis@kernel.org>]

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