* [PATCH] btrfs: join DEV_STATS ioctls to one
@ 2012-06-22 12:30 David Sterba
2012-06-22 13:33 ` Josef Bacik
2012-06-22 16:26 ` Stefan Behrens
0 siblings, 2 replies; 6+ messages in thread
From: David Sterba @ 2012-06-22 12:30 UTC (permalink / raw)
To: chris.mason; +Cc: linux-btrfs, David Sterba, Stefan Behrens
Hi Chris,
please consider this patch for 3.5-rc before it goes final. Thanks.
From: David Sterba <dsterba@suse.cz>
Commit c11d2c236cc260b36 (Btrfs: add ioctl to get and reset the device
stats) introduced two ioctls doing almost the same thing distinguished
by just the ioctl number which encodes "do reset after read". I have
suggested
http://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg16604.html
to implement it via the ioctl args. This hasn't happen, and I think we
should use a more clean way to pass flags and should not waste ioctl
numbers.
CC: Stefan Behrens <sbehrens@giantdisaster.de>
Signed-off-by: David Sterba <dsterba@suse.cz>
---
fs/btrfs/ioctl.c | 16 ++++++++--------
fs/btrfs/ioctl.h | 6 ++++--
fs/btrfs/volumes.c | 5 ++---
fs/btrfs/volumes.h | 3 +--
4 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 0e92e57..670c5d2 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3063,19 +3063,21 @@ static long btrfs_ioctl_scrub_progress(struct btrfs_root *root,
}
static long btrfs_ioctl_get_dev_stats(struct btrfs_root *root,
- void __user *arg, int reset_after_read)
+ void __user *arg)
{
struct btrfs_ioctl_get_dev_stats *sa;
int ret;
- if (reset_after_read && !capable(CAP_SYS_ADMIN))
- return -EPERM;
-
sa = memdup_user(arg, sizeof(*sa));
if (IS_ERR(sa))
return PTR_ERR(sa);
- ret = btrfs_get_dev_stats(root, sa, reset_after_read);
+ if ((sa->flags & BTRFS_DEV_STATS_RESET) && !capable(CAP_SYS_ADMIN)) {
+ kfree(sa);
+ return -EPERM;
+ }
+
+ ret = btrfs_get_dev_stats(root, sa);
if (copy_to_user(arg, sa, sizeof(*sa)))
ret = -EFAULT;
@@ -3473,9 +3475,7 @@ long btrfs_ioctl(struct file *file, unsigned int
case BTRFS_IOC_BALANCE_PROGRESS:
return btrfs_ioctl_balance_progress(root, argp);
case BTRFS_IOC_GET_DEV_STATS:
- return btrfs_ioctl_get_dev_stats(root, argp, 0);
- case BTRFS_IOC_GET_AND_RESET_DEV_STATS:
- return btrfs_ioctl_get_dev_stats(root, argp, 1);
+ return btrfs_ioctl_get_dev_stats(root, argp);
}
return -ENOTTY;
diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
index 497c530..c4089c5 100644
--- a/fs/btrfs/ioctl.h
+++ b/fs/btrfs/ioctl.h
@@ -285,9 +285,13 @@ enum btrfs_dev_stat_values {
BTRFS_DEV_STAT_VALUES_MAX
};
+/* Reset statistics after reading; needs SYS_ADMIN capability */
+#define BTRFS_DEV_STATS_RESET (1ULL << 0)
+
struct btrfs_ioctl_get_dev_stats {
__u64 devid; /* in */
__u64 nr_items; /* in/out */
+ __u64 flags; /* in/out */
/* out values: */
__u64 values[BTRFS_DEV_STAT_VALUES_MAX];
@@ -361,7 +365,5 @@ struct btrfs_ioctl_get_dev_stats {
struct btrfs_ioctl_ino_path_args)
#define BTRFS_IOC_GET_DEV_STATS _IOWR(BTRFS_IOCTL_MAGIC, 52, \
struct btrfs_ioctl_get_dev_stats)
-#define BTRFS_IOC_GET_AND_RESET_DEV_STATS _IOWR(BTRFS_IOCTL_MAGIC, 53, \
- struct btrfs_ioctl_get_dev_stats)
#endif
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 8a3d259..661e6ca 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -4871,8 +4871,7 @@ static void btrfs_dev_stat_print_on_load(struct btrfs_device *dev)
}
int btrfs_get_dev_stats(struct btrfs_root *root,
- struct btrfs_ioctl_get_dev_stats *stats,
- int reset_after_read)
+ struct btrfs_ioctl_get_dev_stats *stats)
{
struct btrfs_device *dev;
struct btrfs_fs_devices *fs_devices = root->fs_info->fs_devices;
@@ -4890,7 +4889,7 @@ int btrfs_get_dev_stats(struct btrfs_root *root,
printk(KERN_WARNING
"btrfs: get dev_stats failed, not yet valid\n");
return -ENODEV;
- } else if (reset_after_read) {
+ } else if (stats->flags & BTRFS_DEV_STATS_RESET) {
for (i = 0; i < BTRFS_DEV_STAT_VALUES_MAX; i++) {
if (stats->nr_items > i)
stats->values[i] =
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 74366f2..e673589 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -292,8 +292,7 @@ struct btrfs_device *btrfs_find_device_for_logical(struct btrfs_root *root,
void btrfs_dev_stat_print_on_error(struct btrfs_device *device);
void btrfs_dev_stat_inc_and_print(struct btrfs_device *dev, int index);
int btrfs_get_dev_stats(struct btrfs_root *root,
- struct btrfs_ioctl_get_dev_stats *stats,
- int reset_after_read);
+ struct btrfs_ioctl_get_dev_stats *stats);
int btrfs_init_dev_stats(struct btrfs_fs_info *fs_info);
int btrfs_run_dev_stats(struct btrfs_trans_handle *trans,
struct btrfs_fs_info *fs_info);
--
1.7.9
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] btrfs: join DEV_STATS ioctls to one
2012-06-22 12:30 [PATCH] btrfs: join DEV_STATS ioctls to one David Sterba
@ 2012-06-22 13:33 ` Josef Bacik
2012-06-22 16:26 ` Stefan Behrens
1 sibling, 0 replies; 6+ messages in thread
From: Josef Bacik @ 2012-06-22 13:33 UTC (permalink / raw)
To: David Sterba; +Cc: Chris L. Mason, linux-btrfs@vger.kernel.org, Stefan Behrens
On 06/22/2012 08:30 AM, David Sterba wrote:
> Hi Chris,
>
> please consider this patch for 3.5-rc before it goes final. Thanks.
>
> From: David Sterba <dsterba@suse.cz>
>
> Commit c11d2c236cc260b36 (Btrfs: add ioctl to get and reset the device
> stats) introduced two ioctls doing almost the same thing distinguished
> by just the ioctl number which encodes "do reset after read". I have
> suggested
>
> http://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg16604.html
>
> to implement it via the ioctl args. This hasn't happen, and I think we
> should use a more clean way to pass flags and should not waste ioctl
> numbers.
>
I agree, pulling into btrfs-next. Thanks,
Josef
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: join DEV_STATS ioctls to one
2012-06-22 12:30 [PATCH] btrfs: join DEV_STATS ioctls to one David Sterba
2012-06-22 13:33 ` Josef Bacik
@ 2012-06-22 16:26 ` Stefan Behrens
2012-06-22 17:02 ` David Sterba
2012-06-22 17:16 ` Josef Bacik
1 sibling, 2 replies; 6+ messages in thread
From: Stefan Behrens @ 2012-06-22 16:26 UTC (permalink / raw)
To: David Sterba; +Cc: chris.mason, linux-btrfs
On Fri, 22 Jun 2012 14:30:39 +0200, David Sterba wrote:
> Hi Chris,
>
> please consider this patch for 3.5-rc before it goes final. Thanks.
>
> From: David Sterba <dsterba@suse.cz>
>
> Commit c11d2c236cc260b36 (Btrfs: add ioctl to get and reset the device
> stats) introduced two ioctls doing almost the same thing distinguished
> by just the ioctl number which encodes "do reset after read". I have
> suggested
>
> http://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg16604.html
>
> to implement it via the ioctl args. This hasn't happen, and I think we
> should use a more clean way to pass flags and should not waste ioctl
> numbers.
>
> CC: Stefan Behrens <sbehrens@giantdisaster.de>
> Signed-off-by: David Sterba <dsterba@suse.cz>
> ---
> fs/btrfs/ioctl.c | 16 ++++++++--------
> fs/btrfs/ioctl.h | 6 ++++--
> fs/btrfs/volumes.c | 5 ++---
> fs/btrfs/volumes.h | 3 +--
> 4 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 0e92e57..670c5d2 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -3063,19 +3063,21 @@ static long btrfs_ioctl_scrub_progress(struct btrfs_root *root,
> }
>
> static long btrfs_ioctl_get_dev_stats(struct btrfs_root *root,
> - void __user *arg, int reset_after_read)
> + void __user *arg)
> {
> struct btrfs_ioctl_get_dev_stats *sa;
> int ret;
>
> - if (reset_after_read && !capable(CAP_SYS_ADMIN))
> - return -EPERM;
> -
> sa = memdup_user(arg, sizeof(*sa));
> if (IS_ERR(sa))
> return PTR_ERR(sa);
>
> - ret = btrfs_get_dev_stats(root, sa, reset_after_read);
> + if ((sa->flags & BTRFS_DEV_STATS_RESET) && !capable(CAP_SYS_ADMIN)) {
> + kfree(sa);
> + return -EPERM;
> + }
> +
> + ret = btrfs_get_dev_stats(root, sa);
>
> if (copy_to_user(arg, sa, sizeof(*sa)))
> ret = -EFAULT;
> @@ -3473,9 +3475,7 @@ long btrfs_ioctl(struct file *file, unsigned int
> case BTRFS_IOC_BALANCE_PROGRESS:
> return btrfs_ioctl_balance_progress(root, argp);
> case BTRFS_IOC_GET_DEV_STATS:
> - return btrfs_ioctl_get_dev_stats(root, argp, 0);
> - case BTRFS_IOC_GET_AND_RESET_DEV_STATS:
> - return btrfs_ioctl_get_dev_stats(root, argp, 1);
> + return btrfs_ioctl_get_dev_stats(root, argp);
> }
>
> return -ENOTTY;
> diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
> index 497c530..c4089c5 100644
> --- a/fs/btrfs/ioctl.h
> +++ b/fs/btrfs/ioctl.h
> @@ -285,9 +285,13 @@ enum btrfs_dev_stat_values {
> BTRFS_DEV_STAT_VALUES_MAX
> };
>
> +/* Reset statistics after reading; needs SYS_ADMIN capability */
> +#define BTRFS_DEV_STATS_RESET (1ULL << 0)
> +
> struct btrfs_ioctl_get_dev_stats {
> __u64 devid; /* in */
> __u64 nr_items; /* in/out */
> + __u64 flags; /* in/out */
>
> /* out values: */
> __u64 values[BTRFS_DEV_STAT_VALUES_MAX];
> @@ -361,7 +365,5 @@ struct btrfs_ioctl_get_dev_stats {
> struct btrfs_ioctl_ino_path_args)
> #define BTRFS_IOC_GET_DEV_STATS _IOWR(BTRFS_IOCTL_MAGIC, 52, \
> struct btrfs_ioctl_get_dev_stats)
> -#define BTRFS_IOC_GET_AND_RESET_DEV_STATS _IOWR(BTRFS_IOCTL_MAGIC, 53, \
> - struct btrfs_ioctl_get_dev_stats)
>
> #endif
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 8a3d259..661e6ca 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -4871,8 +4871,7 @@ static void btrfs_dev_stat_print_on_load(struct btrfs_device *dev)
> }
>
> int btrfs_get_dev_stats(struct btrfs_root *root,
> - struct btrfs_ioctl_get_dev_stats *stats,
> - int reset_after_read)
> + struct btrfs_ioctl_get_dev_stats *stats)
> {
> struct btrfs_device *dev;
> struct btrfs_fs_devices *fs_devices = root->fs_info->fs_devices;
> @@ -4890,7 +4889,7 @@ int btrfs_get_dev_stats(struct btrfs_root *root,
> printk(KERN_WARNING
> "btrfs: get dev_stats failed, not yet valid\n");
> return -ENODEV;
> - } else if (reset_after_read) {
> + } else if (stats->flags & BTRFS_DEV_STATS_RESET) {
> for (i = 0; i < BTRFS_DEV_STAT_VALUES_MAX; i++) {
> if (stats->nr_items > i)
> stats->values[i] =
> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
> index 74366f2..e673589 100644
> --- a/fs/btrfs/volumes.h
> +++ b/fs/btrfs/volumes.h
> @@ -292,8 +292,7 @@ struct btrfs_device *btrfs_find_device_for_logical(struct btrfs_root *root,
> void btrfs_dev_stat_print_on_error(struct btrfs_device *device);
> void btrfs_dev_stat_inc_and_print(struct btrfs_device *dev, int index);
> int btrfs_get_dev_stats(struct btrfs_root *root,
> - struct btrfs_ioctl_get_dev_stats *stats,
> - int reset_after_read);
> + struct btrfs_ioctl_get_dev_stats *stats);
> int btrfs_init_dev_stats(struct btrfs_fs_info *fs_info);
> int btrfs_run_dev_stats(struct btrfs_trans_handle *trans,
> struct btrfs_fs_info *fs_info);
>
I still do not understand your reason and the benefit of your change.
The reset command and the read command are two completely different
operations. Therefore I assigned two different ioctl commands. Then you
can use strace to see which command is sent, and you can also grep the
sources without additional effort.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] btrfs: join DEV_STATS ioctls to one
2012-06-22 16:26 ` Stefan Behrens
@ 2012-06-22 17:02 ` David Sterba
2012-06-22 17:21 ` Stefan Behrens
2012-06-22 17:16 ` Josef Bacik
1 sibling, 1 reply; 6+ messages in thread
From: David Sterba @ 2012-06-22 17:02 UTC (permalink / raw)
To: Stefan Behrens; +Cc: David Sterba, chris.mason, linux-btrfs
On Fri, Jun 22, 2012 at 06:26:44PM +0200, Stefan Behrens wrote:
> I still do not understand your reason and the benefit of your change.
> The reset command and the read command are two completely different
> operations. Therefore I assigned two different ioctl commands. Then you
> can use strace to see which command is sent, and you can also grep the
> sources without additional effort.
I'll try better.
I see the primary purpose to read the device stats. A standalone stats
reset ioctl does not make much sense, so it should go along with reading
the last state and then reset. So far ok.
.From implementation and interface POV, the reset is a hint how I want
to read the stats, not directly the command.
The switch whether to do the reset or not, as currently implemented, has
to set the ioctl number, while I'd expect to set a flag.
Other concerns are about future adding more flags or reading them back
from kernel. I don't have examples, this is what I base on my experience
that such things happen and then I can only scratch my head 'why didn't
I add just one more byte there'. Spare bytes avoid some of backward
compatibility problems.
As for strace, it could be taught to be verbose and descriptive about
ioctls arguments (the 3rd parameter). Quick example is
$ strace stty sane
...
ioctl(0, SNDCTL_TMR_TIMEBASE or SNDRV_TIMER_IOCTL_NEXT_DEVICE or TCGETS, {B38400 opost isig icanon echo ...}) = 0
...
Currently, strace does not know about btrfs-specific ioctls, eg snapshot:
ioctl(3, 0x50009417, 0x7ffff4ae8850) = 0
david
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] btrfs: join DEV_STATS ioctls to one
2012-06-22 17:02 ` David Sterba
@ 2012-06-22 17:21 ` Stefan Behrens
0 siblings, 0 replies; 6+ messages in thread
From: Stefan Behrens @ 2012-06-22 17:21 UTC (permalink / raw)
To: David Sterba, chris.mason, linux-btrfs
On 06/22/2012 19:02 +0200, David Sterba wrote:
> On Fri, Jun 22, 2012 at 06:26:44PM +0200, Stefan Behrens wrote:
>> I still do not understand your reason and the benefit of your change.
>> The reset command and the read command are two completely different
>> operations. Therefore I assigned two different ioctl commands. Then you
>> can use strace to see which command is sent, and you can also grep the
>> sources without additional effort.
>
> I'll try better.
>
> I see the primary purpose to read the device stats. A standalone stats
> reset ioctl does not make much sense, so it should go along with reading
> the last state and then reset. So far ok.
>
> .From implementation and interface POV, the reset is a hint how I want
> to read the stats, not directly the command.
>
> The switch whether to do the reset or not, as currently implemented, has
> to set the ioctl number, while I'd expect to set a flag.
>
> Other concerns are about future adding more flags or reading them back
> from kernel. I don't have examples, this is what I base on my experience
> that such things happen and then I can only scratch my head 'why didn't
> I add just one more byte there'. Spare bytes avoid some of backward
> compatibility problems.
>
> As for strace, it could be taught to be verbose and descriptive about
> ioctls arguments (the 3rd parameter). Quick example is
>
> $ strace stty sane
> ...
> ioctl(0, SNDCTL_TMR_TIMEBASE or SNDRV_TIMER_IOCTL_NEXT_DEVICE or TCGETS, {B38400 opost isig icanon echo ...}) = 0
> ...
>
> Currently, strace does not know about btrfs-specific ioctls, eg snapshot:
>
> ioctl(3, 0x50009417, 0x7ffff4ae8850) = 0
>
>
> david
OK. Then let's do it your way. I will prepare the patch to adapt the
btrfs-progs side, unless you have already created one.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs: join DEV_STATS ioctls to one
2012-06-22 16:26 ` Stefan Behrens
2012-06-22 17:02 ` David Sterba
@ 2012-06-22 17:16 ` Josef Bacik
1 sibling, 0 replies; 6+ messages in thread
From: Josef Bacik @ 2012-06-22 17:16 UTC (permalink / raw)
To: Stefan Behrens; +Cc: David Sterba, Chris L. Mason, linux-btrfs@vger.kernel.org
On 06/22/2012 12:26 PM, Stefan Behrens wrote:
> On Fri, 22 Jun 2012 14:30:39 +0200, David Sterba wrote:
>> Hi Chris,
>>
>> please consider this patch for 3.5-rc before it goes final. Thanks.
>>
>> From: David Sterba <dsterba@suse.cz>
>>
>> Commit c11d2c236cc260b36 (Btrfs: add ioctl to get and reset the device
>> stats) introduced two ioctls doing almost the same thing distinguished
>> by just the ioctl number which encodes "do reset after read". I have
>> suggested
>>
>> http://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg16604.html
>>
>> to implement it via the ioctl args. This hasn't happen, and I think we
>> should use a more clean way to pass flags and should not waste ioctl
>> numbers.
>>
>> CC: Stefan Behrens <sbehrens@giantdisaster.de>
>> Signed-off-by: David Sterba <dsterba@suse.cz>
>> ---
>> fs/btrfs/ioctl.c | 16 ++++++++--------
>> fs/btrfs/ioctl.h | 6 ++++--
>> fs/btrfs/volumes.c | 5 ++---
>> fs/btrfs/volumes.h | 3 +--
>> 4 files changed, 15 insertions(+), 15 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 0e92e57..670c5d2 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -3063,19 +3063,21 @@ static long btrfs_ioctl_scrub_progress(struct btrfs_root *root,
>> }
>>
>> static long btrfs_ioctl_get_dev_stats(struct btrfs_root *root,
>> - void __user *arg, int reset_after_read)
>> + void __user *arg)
>> {
>> struct btrfs_ioctl_get_dev_stats *sa;
>> int ret;
>>
>> - if (reset_after_read && !capable(CAP_SYS_ADMIN))
>> - return -EPERM;
>> -
>> sa = memdup_user(arg, sizeof(*sa));
>> if (IS_ERR(sa))
>> return PTR_ERR(sa);
>>
>> - ret = btrfs_get_dev_stats(root, sa, reset_after_read);
>> + if ((sa->flags & BTRFS_DEV_STATS_RESET) && !capable(CAP_SYS_ADMIN)) {
>> + kfree(sa);
>> + return -EPERM;
>> + }
>> +
>> + ret = btrfs_get_dev_stats(root, sa);
>>
>> if (copy_to_user(arg, sa, sizeof(*sa)))
>> ret = -EFAULT;
>> @@ -3473,9 +3475,7 @@ long btrfs_ioctl(struct file *file, unsigned int
>> case BTRFS_IOC_BALANCE_PROGRESS:
>> return btrfs_ioctl_balance_progress(root, argp);
>> case BTRFS_IOC_GET_DEV_STATS:
>> - return btrfs_ioctl_get_dev_stats(root, argp, 0);
>> - case BTRFS_IOC_GET_AND_RESET_DEV_STATS:
>> - return btrfs_ioctl_get_dev_stats(root, argp, 1);
>> + return btrfs_ioctl_get_dev_stats(root, argp);
>> }
>>
>> return -ENOTTY;
>> diff --git a/fs/btrfs/ioctl.h b/fs/btrfs/ioctl.h
>> index 497c530..c4089c5 100644
>> --- a/fs/btrfs/ioctl.h
>> +++ b/fs/btrfs/ioctl.h
>> @@ -285,9 +285,13 @@ enum btrfs_dev_stat_values {
>> BTRFS_DEV_STAT_VALUES_MAX
>> };
>>
>> +/* Reset statistics after reading; needs SYS_ADMIN capability */
>> +#define BTRFS_DEV_STATS_RESET (1ULL << 0)
>> +
>> struct btrfs_ioctl_get_dev_stats {
>> __u64 devid; /* in */
>> __u64 nr_items; /* in/out */
>> + __u64 flags; /* in/out */
>>
>> /* out values: */
>> __u64 values[BTRFS_DEV_STAT_VALUES_MAX];
>> @@ -361,7 +365,5 @@ struct btrfs_ioctl_get_dev_stats {
>> struct btrfs_ioctl_ino_path_args)
>> #define BTRFS_IOC_GET_DEV_STATS _IOWR(BTRFS_IOCTL_MAGIC, 52, \
>> struct btrfs_ioctl_get_dev_stats)
>> -#define BTRFS_IOC_GET_AND_RESET_DEV_STATS _IOWR(BTRFS_IOCTL_MAGIC, 53, \
>> - struct btrfs_ioctl_get_dev_stats)
>>
>> #endif
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 8a3d259..661e6ca 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -4871,8 +4871,7 @@ static void btrfs_dev_stat_print_on_load(struct btrfs_device *dev)
>> }
>>
>> int btrfs_get_dev_stats(struct btrfs_root *root,
>> - struct btrfs_ioctl_get_dev_stats *stats,
>> - int reset_after_read)
>> + struct btrfs_ioctl_get_dev_stats *stats)
>> {
>> struct btrfs_device *dev;
>> struct btrfs_fs_devices *fs_devices = root->fs_info->fs_devices;
>> @@ -4890,7 +4889,7 @@ int btrfs_get_dev_stats(struct btrfs_root *root,
>> printk(KERN_WARNING
>> "btrfs: get dev_stats failed, not yet valid\n");
>> return -ENODEV;
>> - } else if (reset_after_read) {
>> + } else if (stats->flags & BTRFS_DEV_STATS_RESET) {
>> for (i = 0; i < BTRFS_DEV_STAT_VALUES_MAX; i++) {
>> if (stats->nr_items > i)
>> stats->values[i] =
>> diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
>> index 74366f2..e673589 100644
>> --- a/fs/btrfs/volumes.h
>> +++ b/fs/btrfs/volumes.h
>> @@ -292,8 +292,7 @@ struct btrfs_device *btrfs_find_device_for_logical(struct btrfs_root *root,
>> void btrfs_dev_stat_print_on_error(struct btrfs_device *device);
>> void btrfs_dev_stat_inc_and_print(struct btrfs_device *dev, int index);
>> int btrfs_get_dev_stats(struct btrfs_root *root,
>> - struct btrfs_ioctl_get_dev_stats *stats,
>> - int reset_after_read);
>> + struct btrfs_ioctl_get_dev_stats *stats);
>> int btrfs_init_dev_stats(struct btrfs_fs_info *fs_info);
>> int btrfs_run_dev_stats(struct btrfs_trans_handle *trans,
>> struct btrfs_fs_info *fs_info);
>>
>
> I still do not understand your reason and the benefit of your change.
> The reset command and the read command are two completely different
> operations. Therefore I assigned two different ioctl commands. Then you
> can use strace to see which command is sent, and you can also grep the
> sources without additional effort.
If the difference in behaviour between two different ioctls is indicated
by a single flag to the same function then there is no reason it cannot
be implemented in one ioctl with a flag in the ioctl arguments. I'd
rather not waste ioctl numbers. Thanks,
Josef
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-06-22 17:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-06-22 12:30 [PATCH] btrfs: join DEV_STATS ioctls to one David Sterba
2012-06-22 13:33 ` Josef Bacik
2012-06-22 16:26 ` Stefan Behrens
2012-06-22 17:02 ` David Sterba
2012-06-22 17:21 ` Stefan Behrens
2012-06-22 17:16 ` Josef Bacik
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).