Re: [RFC] Live resize of backing device

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Coly,
thank you for taking the time to reply, I really hope you are feeling
better now.

On Fri, Dec 30, 2022 at 11:41 AM Coly Li <colyli@xxxxxxx> wrote:
>
> On 9/8/22 4:32 PM, Andrea Tomassetti wrote:
> > From 59787372cf21af0b79e895578ae05b6586dfeb09 Mon Sep 17 00:00:00 2001
> > From: Andrea Tomassetti <andrea.tomassetti-opensource@xxxxxxxx>
> > Date: Thu, 8 Sep 2022 09:47:55 +0200
> > Subject: [PATCH] bcache: Add support for live resize of backing devices
> >
> > Signed-off-by: Andrea Tomassetti <andrea.tomassetti-opensource@xxxxxxxx>
>
> Hi Andrea,
>
> I am just recovered from Omicron, and able to reply email. Let me place
> my comments inline.
>
>
> > ---
> > Hi Coly,
> > Here is the first version of the patch. There are some points I noted
> > down
> > that I would like to discuss with you:
> >  - I found it pretty convenient to hook the call of the new added
> > function
> >    inside the `register_bcache`. In fact, every time (at least from my
> >    understandings) a disk changes size, it will trigger a new probe and,
> >    thus, `register_bcache` will be triggered. The only inconvenient
> >    is that, in case of success, the function will output
>
> The resize should be triggered manually, and not to do it automatically.
>
> You may create a sysfs file under the cached device's directory, name it
> as "extend_size" or something else you think better.
>
> Then the sysadmin may extend the cached device size explicitly on a
> predictable time.
>
> > `error: capacity changed` even if it's not really an error.
> >  - I'm using `kvrealloc`, introduced in kernel version 5.15, to resize
> >    `stripe_sectors_dirty` and `full_dirty_stripes`. It shouldn't be a
> >    problem, right?
> >  - There is some reused code between this new function and
> >    `bcache_device_init`. Maybe I can move `const size_t max_stripes` to
> >    a broader scope or make it a macro, what do you think?
> >
> > Thank you very much,
> > Andrea
> >
> >  drivers/md/bcache/super.c | 75 ++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 74 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> > index ba3909bb6bea..9a77caf2a18f 100644
> > --- a/drivers/md/bcache/super.c
> > +++ b/drivers/md/bcache/super.c
> > @@ -2443,6 +2443,76 @@ static bool bch_is_open(dev_t dev)
> >      return bch_is_open_cache(dev) || bch_is_open_backing(dev);
> >  }
> >
> > +static bool bch_update_capacity(dev_t dev)
> > +{
> > +    const size_t max_stripes = min_t(size_t, INT_MAX,
> > +                     SIZE_MAX / sizeof(atomic_t));
> > +
> > +    uint64_t n, n_old;
> > +    int nr_stripes_old;
> > +    bool res = false;
> > +
> > +    struct bcache_device *d;
> > +    struct cache_set *c, *tc;
> > +    struct cached_dev *dcp, *t, *dc = NULL;
> > +
> > +    uint64_t parent_nr_sectors;
> > +
> > +    list_for_each_entry_safe(c, tc, &bch_cache_sets, list)
> > +        list_for_each_entry_safe(dcp, t, &c->cached_devs, list)
> > +            if (dcp->bdev->bd_dev == dev) {
> > +                dc = dcp;
> > +                goto dc_found;
> > +            }
> > +
> > +dc_found:
> > +    if (!dc)
> > +        return false;
> > +
> > +    parent_nr_sectors = bdev_nr_sectors(dc->bdev) - dc->sb.data_offset;
> > +
> > +    if (parent_nr_sectors == bdev_nr_sectors(dc->disk.disk->part0))
> > +        return false;
> > +
>
> The above code only handles whole disk using as cached device. If a
> partition of a hard drive is used as cache device, and there are other
> data after this partition, such condition should be handled as well. So
> far I am fine to only extend size when using the whole hard drive as
> cached device, but more code is necessary to check and only permits
> size-extend for such condition.
I don't understand if there's a misalignment here so let me be more
clear: this patch is intended to support the live resize of *backing
devices*, is this what you mean with "cached device"?
When I was working on the patch I didn't consider the possibility of
live resizing the cache devices, but it should be trivial.
So, as far as I understand, a partition cannot be used as a backing
device, correct? The whole disk should be used as a backing device,
that's why I'm checking and that's why this check should be correct.
Am I wrong?

>
> > +    if (!set_capacity_and_notify(dc->disk.disk, parent_nr_sectors))
> > +        return false;
>
> The above code should be done when all rested are set.
>
>
> > +
> > +    d = &dc->disk;
> > +
> > +    /* Force cached device sectors re-calc */
> > +    calc_cached_dev_sectors(d->c);
>
> Here c->cached_dev_sectors might be changed, if any of the following
> steps fails, it should be restored to previous value.
>
>
> > +
> > +    /* Block writeback thread */
> > +    down_write(&dc->writeback_lock);
> > +    nr_stripes_old = d->nr_stripes;
> > +    n = DIV_ROUND_UP_ULL(parent_nr_sectors, d->stripe_size);
> > +    if (!n || n > max_stripes) {
> > +        pr_err("nr_stripes too large or invalid: %llu (start sector
> > beyond end of disk?)\n",
> > +            n);
> > +        goto unblock_and_exit;
> > +    }
> > +    d->nr_stripes = n;
> > +
> > +    n = d->nr_stripes * sizeof(atomic_t);
> > +    n_old = nr_stripes_old * sizeof(atomic_t);
> > +    d->stripe_sectors_dirty = kvrealloc(d->stripe_sectors_dirty, n_old,
> > +        n, GFP_KERNEL);
> > +    if (!d->stripe_sectors_dirty)
> > +        goto unblock_and_exit;
> > +
> > +    n = BITS_TO_LONGS(d->nr_stripes) * sizeof(unsigned long);
> > +    n_old = BITS_TO_LONGS(nr_stripes_old) * sizeof(unsigned long);
> > +    d->full_dirty_stripes = kvrealloc(d->full_dirty_stripes, n_old,
> > n, GFP_KERNEL);
> > +    if (!d->full_dirty_stripes)
> > +        goto unblock_and_exit;
>
> If kvrealloc() fails and NULL set to d->full_dirty_sripes, the previous
> array should be restored.
>
> > +
> > +    res = true;
> > +
> > +unblock_and_exit:
> > +    up_write(&dc->writeback_lock);
> > +    return res;
> > +}
> > +
>
> I didn't test the patch, from the first glance, I feel the failure
> handling should restore all previous values, otherwise the cache device
> may be in a non-consistent state when extend size fails.
>
Completely agree with you on this point. I changed it locally and, as
soon as we agree on the other points I'll send a newer version of the
patch.
>
> >  struct async_reg_args {
> >      struct delayed_work reg_work;
> >      char *path;
> > @@ -2569,7 +2639,10 @@ static ssize_t register_bcache(struct kobject
> > *k, struct kobj_attribute *attr,
> >              mutex_lock(&bch_register_lock);
> >              if (lookup_bdev(strim(path), &dev) == 0 &&
> >                  bch_is_open(dev))
> > -                err = "device already registered";
> > +                if (bch_update_capacity(dev))
> > +                    err = "capacity changed";
> > +                else
> > +                    err = "device already registered";
>
>
> As I said, it should be a separated write-only sysfile under the cache
> device's directory.
Can I ask why you don't like the automatic resize way? Why should the
resize be manual?
Someone needs to trigger the block device resize, so shouldn't that be enough?

Andrea
>
>
> > else
> >                  err = "device busy";
> >              mutex_unlock(&bch_register_lock);
> > --
> > 2.37.3
> >
>



[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Linux ARM Kernel]     [Linux Filesystem Development]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux