On Sat 03-08-19 07:01:53, Tejun Heo wrote:
> There currently is no way to universally identify and lookup a bdi
> without holding a reference and pointer to it. This patch adds an
> non-recycling bdi->id and implements bdi_get_by_id() which looks up
> bdis by their ids. This will be used by memcg foreign inode flushing.
>
> I left bdi_list alone for simplicity and because while rb_tree does
> support rcu assignment it doesn't seem to guarantee lossless walk when
> walk is racing aginst tree rebalance operations.
>
> Signed-off-by: Tejun Heo <tj@xxxxxxxxxx>
The patch looks good to me. You can add:
Reviewed-by: Jan Kara <jack@xxxxxxx>
Although I would note that here you take effort not to recycle bdi->id so
that you don't flush wrong devices while in patch 4 you take pretty lax
approach to feeding garbage into the writeback system. So these two don't
quite match to me...
Honza
> ---
> include/linux/backing-dev-defs.h | 2 +
> include/linux/backing-dev.h | 1 +
> mm/backing-dev.c | 65 +++++++++++++++++++++++++++++++-
> 3 files changed, 66 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/backing-dev-defs.h b/include/linux/backing-dev-defs.h
> index 8fb740178d5d..1075f2552cfc 100644
> --- a/include/linux/backing-dev-defs.h
> +++ b/include/linux/backing-dev-defs.h
> @@ -185,6 +185,8 @@ struct bdi_writeback {
> };
>
> struct backing_dev_info {
> + u64 id;
> + struct rb_node rb_node; /* keyed by ->id */
> struct list_head bdi_list;
> unsigned long ra_pages; /* max readahead in PAGE_SIZE units */
> unsigned long io_pages; /* max allowed IO size */
> diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
> index 02650b1253a2..84cdcfbc763f 100644
> --- a/include/linux/backing-dev.h
> +++ b/include/linux/backing-dev.h
> @@ -24,6 +24,7 @@ static inline struct backing_dev_info *bdi_get(struct backing_dev_info *bdi)
> return bdi;
> }
>
> +struct backing_dev_info *bdi_get_by_id(u64 id);
> void bdi_put(struct backing_dev_info *bdi);
>
> __printf(2, 3)
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index e8e89158adec..4a8816e0b8d4 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -1,6 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0-only
>
> #include <linux/wait.h>
> +#include <linux/rbtree.h>
> #include <linux/backing-dev.h>
> #include <linux/kthread.h>
> #include <linux/freezer.h>
> @@ -22,10 +23,12 @@ EXPORT_SYMBOL_GPL(noop_backing_dev_info);
> static struct class *bdi_class;
>
> /*
> - * bdi_lock protects updates to bdi_list. bdi_list has RCU reader side
> - * locking.
> + * bdi_lock protects bdi_tree and updates to bdi_list. bdi_list has RCU
> + * reader side locking.
> */
> DEFINE_SPINLOCK(bdi_lock);
> +static u64 bdi_id_cursor;
> +static struct rb_root bdi_tree = RB_ROOT;
> LIST_HEAD(bdi_list);
>
> /* bdi_wq serves all asynchronous writeback tasks */
> @@ -859,9 +862,58 @@ struct backing_dev_info *bdi_alloc_node(gfp_t gfp_mask, int node_id)
> }
> EXPORT_SYMBOL(bdi_alloc_node);
>
> +struct rb_node **bdi_lookup_rb_node(u64 id, struct rb_node **parentp)
> +{
> + struct rb_node **p = &bdi_tree.rb_node;
> + struct rb_node *parent = NULL;
> + struct backing_dev_info *bdi;
> +
> + lockdep_assert_held(&bdi_lock);
> +
> + while (*p) {
> + parent = *p;
> + bdi = rb_entry(parent, struct backing_dev_info, rb_node);
> +
> + if (bdi->id > id)
> + p = &(*p)->rb_left;
> + else if (bdi->id < id)
> + p = &(*p)->rb_right;
> + else
> + break;
> + }
> +
> + if (parentp)
> + *parentp = parent;
> + return p;
> +}
> +
> +/**
> + * bdi_get_by_id - lookup and get bdi from its id
> + * @id: bdi id to lookup
> + *
> + * Find bdi matching @id and get it. Returns NULL if the matching bdi
> + * doesn't exist or is already unregistered.
> + */
> +struct backing_dev_info *bdi_get_by_id(u64 id)
> +{
> + struct backing_dev_info *bdi = NULL;
> + struct rb_node **p;
> +
> + spin_lock_irq(&bdi_lock);
> + p = bdi_lookup_rb_node(id, NULL);
> + if (*p) {
> + bdi = rb_entry(*p, struct backing_dev_info, rb_node);
> + bdi_get(bdi);
> + }
> + spin_unlock_irq(&bdi_lock);
> +
> + return bdi;
> +}
> +
> int bdi_register_va(struct backing_dev_info *bdi, const char *fmt, va_list args)
> {
> struct device *dev;
> + struct rb_node *parent, **p;
>
> if (bdi->dev) /* The driver needs to use separate queues per device */
> return 0;
> @@ -877,7 +929,15 @@ int bdi_register_va(struct backing_dev_info *bdi, const char *fmt, va_list args)
> set_bit(WB_registered, &bdi->wb.state);
>
> spin_lock_bh(&bdi_lock);
> +
> + bdi->id = ++bdi_id_cursor;
> +
> + p = bdi_lookup_rb_node(bdi->id, &parent);
> + rb_link_node(&bdi->rb_node, parent, p);
> + rb_insert_color(&bdi->rb_node, &bdi_tree);
> +
> list_add_tail_rcu(&bdi->bdi_list, &bdi_list);
> +
> spin_unlock_bh(&bdi_lock);
>
> trace_writeback_bdi_register(bdi);
> @@ -918,6 +978,7 @@ EXPORT_SYMBOL(bdi_register_owner);
> static void bdi_remove_from_list(struct backing_dev_info *bdi)
> {
> spin_lock_bh(&bdi_lock);
> + rb_erase(&bdi->rb_node, &bdi_tree);
> list_del_rcu(&bdi->bdi_list);
> spin_unlock_bh(&bdi_lock);
>
> --
> 2.17.1
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
