Re: [PATCH] btrfs: join DEV_STATS ioctls to one

[Stefan Behrens <sbehrens@giantdisaster.de>]

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.