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
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > > > davidOK. Then let''s do it your way. I will prepare the patch to adapt the btrfs-progs side, unless you have already created one. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html