Btrfs devel - Sep 2012 - R: [PATCH 2/2] Btrfs-progs: add mount-option command

Hi Seto,

please could you update also the man page too ? 
Why it was not provided a way to clear a *single* flag ? To me it seems a bit 
too long to clear all the flag (btrfs mount-option clear) and then set the 
right one.

As user interface I suggest something like chmod:

   btrfs mount-option set +ssd,skip_balance -nodatacow /dev/sdX

or

   btrfs mount-option set =ssd,skip_balance,nodatacow /dev/sdX


Finally I have some small concern about two macro (see below)
>----Messaggio originale----
>Da: seto.hidetoshi@jp.fujitsu.com
>Data: 18/09/2012 3.30
>A: <linux-btrfs@vger.kernel.org>
>Ogg: [PATCH 2/2] Btrfs-progs: add mount-option command
>
[...]
>+/*
>+ * Flags for mount options.
>+ *
>+ * Note: don''t forget to add new options to btrfs_show_options()
>+ */
>+#define BTRFS_MOUNT_NODATASUM		(1 << 0)
>+#define BTRFS_MOUNT_NODATACOW		(1 << 1)
>+#define BTRFS_MOUNT_NOBARRIER		(1 << 2)
>+#define BTRFS_MOUNT_SSD			(1 << 3)
>+#define BTRFS_MOUNT_DEGRADED		(1 << 4)
>+#define BTRFS_MOUNT_COMPRESS		(1 << 5)
>+#define BTRFS_MOUNT_NOTREELOG           (1 << 6)
>+#define BTRFS_MOUNT_FLUSHONCOMMIT       (1 << 7)
>+#define BTRFS_MOUNT_SSD_SPREAD		(1 << 8)
>+#define BTRFS_MOUNT_NOSSD		(1 << 9)
>+#define BTRFS_MOUNT_DISCARD		(1 << 10)
>+#define BTRFS_MOUNT_FORCE_COMPRESS      (1 << 11)
>+#define BTRFS_MOUNT_SPACE_CACHE		(1 << 12)
>+#define BTRFS_MOUNT_CLEAR_CACHE		(1 << 13)
>+#define BTRFS_MOUNT_USER_SUBVOL_RM_ALLOWED (1 << 14)
>+#define BTRFS_MOUNT_ENOSPC_DEBUG	 (1 << 15)
>+#define BTRFS_MOUNT_AUTO_DEFRAG		(1 << 16)
>+#define BTRFS_MOUNT_INODE_MAP_CACHE	(1 << 17)
>+#define BTRFS_MOUNT_RECOVERY		(1 << 18)
>+#define BTRFS_MOUNT_SKIP_BALANCE	(1 << 19)
>+#define BTRFS_MOUNT_CHECK_INTEGRITY	(1 << 20)
>+#define BTRFS_MOUNT_CHECK_INTEGRITY_INCLUDING_EXTENT_DATA (1 << 21)
>+#define BTRFS_MOUNT_PANIC_ON_FATAL_ERROR	(1 << 22)
>+
>+#define btrfs_clear_opt(o, opt)		((o) &= ~BTRFS_MOUNT_##opt)
>+#define btrfs_set_opt(o, opt)		((o) |= BTRFS_MOUNT_##opt)
>+#define btrfs_test_opt(root, opt)	((root)->fs_info->mount_opt& \
>+					 BTRFS_MOUNT_##opt)
I think that the macro names are too generic, I suggest to rename the three 
macros above as

#define btrfs_mount_XXXX_opt

Also, the last one should be 

#define btrfs_test_opt(o, opt)	( o & BTRFS_MOUNT_##opt)

Or better the other two

#define btrfs_mount_clear_opt(root, opt)		(((root)->fs_info->mount_opt)
&=
~BTRFS_MOUNT_##opt)
#define btrfs_mount_set_opt(root, opt)		(((root)->fs_info->mount_opt) |= 
BTRFS_MOUNT_##opt)



>-- 
>1.7.7.6
>
>
>--
>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
>

--
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

>Da: rm@romanrm.ru
>Data: 18/09/2012 6.19
>A: <miaox@cn.fujitsu.com>
>Cc: "Hidetoshi Seto"<seto.hidetoshi@jp.fujitsu.com>,
<linux-btrfs@vger.kernel.
org>
>Ogg: Re: [PATCH 2/2] Btrfs-progs: add mount-option command
>
>On Tue, 18 Sep 2012 10:31:41 +0800
>Miao Xie <miaox@cn.fujitsu.com> wrote:
>
>> On tue, 18 Sep 2012 10:30:17 +0900, Hidetoshi Seto wrote:
>> > This patch adds mount-option command.
>> > The command can set/get default mount options.
>> > Now, the command can set/get 24 options.
>> > These options are equal to mount options which store
>> > in fs_info/mount-opt.
>> 
>> I don''t think we need implement a separate command to do this,
>> we can add it into btrfstune just like ext3/4. If so, the users
>> who used ext3/4 before can be familiar with btrfs command as soon
>> as possible.
>
>btrfstune currently only does one thing:
>
>$ sudo btrfstune
>usage: btrfstune [options] device
>	-S value	enable/disable seeding
>
>To me it''d seem more logical the other way, why not move this
operation to
the
>base "btrfs" utility under some command, and remove
"btrfstune" completely.
I fully agree. It doesn''t make sense to have btrfstune as separate
command.
Its functionality should be integrate in Hidetoshi''s patch: at the end
both
clear/set some flags.

I am not happy about the "btrfs property *" syntax, however the Hugo 
suggestion is right: these flags (with the seed one) are filesystem properties, 
and should be integrated in the Alexander work... However I don''t know
the
status if its patches...

>
>-- 
>With respect,
>Roman
>
>~~~~~~~~~~~~~~~~~~~~~~~~~~~
>"Stallman had a printer,
>with code he could not see.
>So he began to tinker,
>and set the software free."
>

--
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

(2012/09/18 19:03), Goffredo Baroncelli <kreijack@libero.it>
wrote:
> Hi Seto,
> 
> please could you update also the man page too ? 
Sure. I''ll update it next time.
> Why it was not provided a way to clear a *single* flag ? To me it seems a
bit
> too long to clear all the flag (btrfs mount-option clear) and then set the 
> right one.
Good point. We should have a way to clear defaults, by "clear" or
"set none" or so on. 
> 
> As user interface I suggest something like chmod:
> 
>    btrfs mount-option set +ssd,skip_balance -nodatacow /dev/sdX
> 
> or
> 
>    btrfs mount-option set =ssd,skip_balance,nodatacow /dev/sdX
Interesting. I guess you mean that "=<options>" will reset
defaults
by specified options, while "+<options>" and
"-<options>" will be
used to add/delete option to/from current defaults.
> 
> Finally I have some small concern about two macro (see below)
> 
>> ----Messaggio originale----
>> Da: seto.hidetoshi@jp.fujitsu.com
>> Data: 18/09/2012 3.30
>> A: <linux-btrfs@vger.kernel.org>
>> Ogg: [PATCH 2/2] Btrfs-progs: add mount-option command
>>
> [...]
>> +/*
>> + * Flags for mount options.
>> + *
>> + * Note: don''t forget to add new options to
btrfs_show_options()
>> + */
>> +#define BTRFS_MOUNT_NODATASUM		(1 << 0)
>> +#define BTRFS_MOUNT_NODATACOW		(1 << 1)
>> +#define BTRFS_MOUNT_NOBARRIER		(1 << 2)
>> +#define BTRFS_MOUNT_SSD			(1 << 3)
>> +#define BTRFS_MOUNT_DEGRADED		(1 << 4)
>> +#define BTRFS_MOUNT_COMPRESS		(1 << 5)
>> +#define BTRFS_MOUNT_NOTREELOG           (1 << 6)
>> +#define BTRFS_MOUNT_FLUSHONCOMMIT       (1 << 7)
>> +#define BTRFS_MOUNT_SSD_SPREAD		(1 << 8)
>> +#define BTRFS_MOUNT_NOSSD		(1 << 9)
>> +#define BTRFS_MOUNT_DISCARD		(1 << 10)
>> +#define BTRFS_MOUNT_FORCE_COMPRESS      (1 << 11)
>> +#define BTRFS_MOUNT_SPACE_CACHE		(1 << 12)
>> +#define BTRFS_MOUNT_CLEAR_CACHE		(1 << 13)
>> +#define BTRFS_MOUNT_USER_SUBVOL_RM_ALLOWED (1 << 14)
>> +#define BTRFS_MOUNT_ENOSPC_DEBUG	 (1 << 15)
>> +#define BTRFS_MOUNT_AUTO_DEFRAG		(1 << 16)
>> +#define BTRFS_MOUNT_INODE_MAP_CACHE	(1 << 17)
>> +#define BTRFS_MOUNT_RECOVERY		(1 << 18)
>> +#define BTRFS_MOUNT_SKIP_BALANCE	(1 << 19)
>> +#define BTRFS_MOUNT_CHECK_INTEGRITY	(1 << 20)
>> +#define BTRFS_MOUNT_CHECK_INTEGRITY_INCLUDING_EXTENT_DATA (1 <<
21)
>> +#define BTRFS_MOUNT_PANIC_ON_FATAL_ERROR	(1 << 22)
>> +
>> +#define btrfs_clear_opt(o, opt)		((o) &= ~BTRFS_MOUNT_##opt)
>> +#define btrfs_set_opt(o, opt)		((o) |= BTRFS_MOUNT_##opt)
>> +#define btrfs_test_opt(root, opt)
((root)->fs_info->mount_opt& \
>> +					 BTRFS_MOUNT_##opt)
> 
> I think that the macro names are too generic, I suggest to rename the three
> macros above as
> 
> #define btrfs_mount_XXXX_opt
> 
> Also, the last one should be 
> 
> #define btrfs_test_opt(o, opt)	( o & BTRFS_MOUNT_##opt)
> 
> Or better the other two
> 
> #define btrfs_mount_clear_opt(root, opt)	
(((root)->fs_info->mount_opt) &=
> ~BTRFS_MOUNT_##opt)
> #define btrfs_mount_set_opt(root, opt)		(((root)->fs_info->mount_opt)
|=
> BTRFS_MOUNT_##opt)
Right. I''ll fix it.


Thanks,
H.Seto

--
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 Tue, Sep 18, 2012 at 12:03:47PM +0200, Goffredo Baroncelli
<kreijack@libero.it> wrote:
> Why it was not provided a way to clear a *single* flag ? To me it seems a
bit
> too long to clear all the flag (btrfs mount-option clear) and then set the 
> right one.
> 
> As user interface I suggest something like chmod:
> 
>    btrfs mount-option set +ssd,skip_balance -nodatacow /dev/sdX
> 
> or
> 
>    btrfs mount-option set =ssd,skip_balance,nodatacow /dev/sdX
I agree with the +/-/= syntax, it''s quite intuitive and already seen
(chattr).

I''d drop the ''set'' command here, with some options
specified it''s
implied.

Without any it''s ''get''

  btrfs mount-option /dev/...

And the ''clear'' subcommand could be also dropped with this
syntax

  btrfs mount-option = /dev/...

:)


BTW, this modifies the defaults only for the unmounted filesystem, but
the same should be possible for a mounted one in the future.

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