Re: [PATCH 3/3] btrfs: add more information for balance

Goldwyn Rodrigues
2020-08-05T20:09:45Z

On 15:17 05/08, Nikolay Borisov wrote:
>
>
> On 3.08.20 г. 23:29 ч., Goldwyn Rodrigues wrote:
> > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> >
> > Include information about the state of the balance and expected,
> > considered and completed statistics.
> >
> > Q: I am not sure of the cancelled state, and stopping seemed more
> > appropriate since it was a transient state to cancelling the operation.
> > Would you prefer to call it cancelled?
> >
> > This information is not used by user space as of now. We could use it
> > for "btrfs balance status" or ignore this patch for inclusion at all.
> >
> > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > ---
> > fs/btrfs/sysfs.c | 29 ++++++++++++++++++++++++++++-
> > 1 file changed, 28 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c
> > index 71950c121588..001a7ae375d0 100644
> > --- a/fs/btrfs/sysfs.c
> > +++ b/fs/btrfs/sysfs.c
> > @@ -808,6 +808,33 @@ static ssize_t btrfs_checksum_show(struct kobject *kobj,
> >
> > BTRFS_ATTR(, checksum, btrfs_checksum_show);
> >
> > +static ssize_t btrfs_balance_show(struct btrfs_fs_info *fs_info, char *buf)
> > +{
> > + ssize_t ret = 0;
> > + struct btrfs_balance_control *bctl;
> > +
> > + ret += scnprintf(buf, PAGE_SIZE, "balance\n");
> > + spin_lock(&fs_info->balance_lock);
> > + bctl = fs_info->balance_ctl;
> > + if (!bctl) {
> > + ret += scnprintf(buf + ret, PAGE_SIZE - ret,
> > + "State: stopping\n");
>
> nit: Shouldn't this be "stopped"?

I named it stopping because it was in the phase where the request had
come in but it had not completed the stop. This phase lasts for a couple
of hundreds of milliseconds in my test environment.

>
> > + goto out;
> > + }
> > + if (test_bit(BTRFS_FS_BALANCE_RUNNING, &fs_info->flags))
> > + ret += scnprintf(buf + ret, PAGE_SIZE - ret,
> > + "State: running\n");
> > + else
> > + ret += scnprintf(buf + ret, PAGE_SIZE - ret,
> > + "State: paused\n");
> > + ret += scnprintf(buf + ret, PAGE_SIZE - ret, "%llu %llu %llu\n",
> > + bctl->stat.expected, bctl->stat.considered,
> > + bctl->stat.completed);
> > +out:
> > + spin_unlock(&fs_info->balance_lock);
> > + return ret;
> > +}
>
> Technically I don't see anything wrong with this, however I got reminded
> of the following text from sysfs documentation:
>
> "
> Mixing types, expressing multiple lines of data, and doing fancy
>
> formatting of data is heavily frowned upon. Doing these things may get
>
> you publicly humiliated and your code rewritten without notice.
> "
>

Yes, I think it is best to drop this. This information can be obtained
by ioctls as well.

--
Goldwyn