diff mbox series

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

Message ID 20200803202913.15913-4-rgoldwyn@suse.de
State New, archived
Headers show
Series btrfs: Enumerate and export exclusive operations | expand

Commit Message

Goldwyn Rodrigues Aug. 3, 2020, 8:29 p.m. UTC
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(-)

Comments

Nikolay Borisov Aug. 5, 2020, 12:17 p.m. UTC | #1
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:
>
Goldwyn Rodrigues Aug. 5, 2020, 4:29 p.m. UTC | #2
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.
diff mbox series

Patch

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");
+		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;
+}
+
 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: