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

[Nikolay Borisov <nborisov@suse.com>]

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"?

> +		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.
"

> +
>  static ssize_t btrfs_exclusive_operation_show(struct kobject *kobj,
>  		struct kobj_attribute *a, char *buf)
>  {
> @@ -816,7 +843,7 @@ static ssize_t btrfs_exclusive_operation_show(struct kobject *kobj,
>  		case  BTRFS_EXCLOP_NONE:
>  			return scnprintf(buf, PAGE_SIZE, "none\n");
>  		case BTRFS_EXCLOP_BALANCE:
> -			return scnprintf(buf, PAGE_SIZE, "balance\n");
> +			return btrfs_balance_show(fs_info, buf);
>  		case BTRFS_EXCLOP_DEV_ADD:
>  			return scnprintf(buf, PAGE_SIZE, "device add\n");
>  		case BTRFS_EXCLOP_DEV_REPLACE:
>