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

[Goldwyn Rodrigues <rgoldwyn@suse.de>]

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