| Message ID | 20200803202913.15913-4-rgoldwyn@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Enumerate and export exclusive operations | expand |
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: >
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 --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: