Re: [PATCH 1/9] btrfs: switch BTRFS_FS_STATE_* to enums
On Tue, Nov 27, 2018 at 04:24:03PM -0800, Omar Sandoval wrote:
> On Tue, Nov 27, 2018 at 08:53:41PM +0100, David Sterba wrote:
> > We can use simple enum for values that are not part of on-disk format:
> > global filesystem states.
>
> Reviewed-by: Omar Sandoval
>
> Some typos/wording suggestions below.
>
> > Signed-off-by: David Sterba
> > ---
> > fs/btrfs/ctree.h | 25 +++--
> > 1 file changed, 19 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > index a98507fa9192..f82ec5e41b0c 100644
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -109,13 +109,26 @@ static inline unsigned long btrfs_chunk_item_size(int
> > num_stripes)
> > }
> >
> > /*
> > - * File system states
> > + * Runtime (in-memory) states of filesystem
> > */
> > -#define BTRFS_FS_STATE_ERROR 0
> > -#define BTRFS_FS_STATE_REMOUNTING 1
> > -#define BTRFS_FS_STATE_TRANS_ABORTED 2
> > -#define BTRFS_FS_STATE_DEV_REPLACING 3
> > -#define BTRFS_FS_STATE_DUMMY_FS_INFO 4
> > +enum {
> > + /* Global indicator of serious filesysystem errors */
>
> filesysystem -> filesystem
>
> > + BTRFS_FS_STATE_ERROR,
> > + /*
> > +* Filesystem is being remounted, allow to skip some operations, like
> > +* defrag
> > +*/
> > + BTRFS_FS_STATE_REMOUNTING,
> > + /* Track if the transaction abort has been reported */
>
> Which one is "the" transaction abort? This gives me the impression that
> this is a flag on the transaction, but it's actually filesystem state.
> Maybe "Track if a transaction abort has been reported on this
> filesystem"?
Your wording is more clear and I've used it in the patch.
> > + BTRFS_FS_STATE_TRANS_ABORTED,
> > + /*
> > +* Indicate that replace source or target device state is changed and
> > +* allow to block bio operations
> > +*/
>
> Again, this makes it sound like it's device state, but it's actually
> filesystem state. How about "Bio operations should be blocked on this
> filesystem because a source or target device is being destroyed as part
> of a device replace"?
Same. Thanks.
Re: [PATCH 1/9] btrfs: switch BTRFS_FS_STATE_* to enums
Looks good, Reviewed-by: Johannes Thumshirn -- Johannes ThumshirnSUSE Labs jthumsh...@suse.de+49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
Re: [PATCH 1/9] btrfs: switch BTRFS_FS_STATE_* to enums
On 2018/11/28 上午3:53, David Sterba wrote:
> We can use simple enum for values that are not part of on-disk format:
> global filesystem states.
>
> Signed-off-by: David Sterba
Good comment.
Reviewed-by: Qu Wenruo
Thanks,
Qu
> ---
> fs/btrfs/ctree.h | 25 +++--
> 1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index a98507fa9192..f82ec5e41b0c 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -109,13 +109,26 @@ static inline unsigned long btrfs_chunk_item_size(int
> num_stripes)
> }
>
> /*
> - * File system states
> + * Runtime (in-memory) states of filesystem
> */
> -#define BTRFS_FS_STATE_ERROR 0
> -#define BTRFS_FS_STATE_REMOUNTING1
> -#define BTRFS_FS_STATE_TRANS_ABORTED 2
> -#define BTRFS_FS_STATE_DEV_REPLACING 3
> -#define BTRFS_FS_STATE_DUMMY_FS_INFO 4
> +enum {
> + /* Global indicator of serious filesysystem errors */
> + BTRFS_FS_STATE_ERROR,
> + /*
> + * Filesystem is being remounted, allow to skip some operations, like
> + * defrag
> + */
> + BTRFS_FS_STATE_REMOUNTING,
> + /* Track if the transaction abort has been reported */
> + BTRFS_FS_STATE_TRANS_ABORTED,
> + /*
> + * Indicate that replace source or target device state is changed and
> + * allow to block bio operations
> + */
> + BTRFS_FS_STATE_DEV_REPLACING,
> + /* The btrfs_fs_info created for self-tests */
> + BTRFS_FS_STATE_DUMMY_FS_INFO,
> +};
>
> #define BTRFS_BACKREF_REV_MAX256
> #define BTRFS_BACKREF_REV_SHIFT 56
>
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 1/9] btrfs: switch BTRFS_FS_STATE_* to enums
On Tue, Nov 27, 2018 at 08:53:41PM +0100, David Sterba wrote:
> We can use simple enum for values that are not part of on-disk format:
> global filesystem states.
Reviewed-by: Omar Sandoval
Some typos/wording suggestions below.
> Signed-off-by: David Sterba
> ---
> fs/btrfs/ctree.h | 25 +++--
> 1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index a98507fa9192..f82ec5e41b0c 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -109,13 +109,26 @@ static inline unsigned long btrfs_chunk_item_size(int
> num_stripes)
> }
>
> /*
> - * File system states
> + * Runtime (in-memory) states of filesystem
> */
> -#define BTRFS_FS_STATE_ERROR 0
> -#define BTRFS_FS_STATE_REMOUNTING1
> -#define BTRFS_FS_STATE_TRANS_ABORTED 2
> -#define BTRFS_FS_STATE_DEV_REPLACING 3
> -#define BTRFS_FS_STATE_DUMMY_FS_INFO 4
> +enum {
> + /* Global indicator of serious filesysystem errors */
filesysystem -> filesystem
> + BTRFS_FS_STATE_ERROR,
> + /*
> + * Filesystem is being remounted, allow to skip some operations, like
> + * defrag
> + */
> + BTRFS_FS_STATE_REMOUNTING,
> + /* Track if the transaction abort has been reported */
Which one is "the" transaction abort? This gives me the impression that
this is a flag on the transaction, but it's actually filesystem state.
Maybe "Track if a transaction abort has been reported on this
filesystem"?
> + BTRFS_FS_STATE_TRANS_ABORTED,
> + /*
> + * Indicate that replace source or target device state is changed and
> + * allow to block bio operations
> + */
Again, this makes it sound like it's device state, but it's actually
filesystem state. How about "Bio operations should be blocked on this
filesystem because a source or target device is being destroyed as part
of a device replace"?
> + BTRFS_FS_STATE_DEV_REPLACING,
> + /* The btrfs_fs_info created for self-tests */
> + BTRFS_FS_STATE_DUMMY_FS_INFO,
> +};
>
> #define BTRFS_BACKREF_REV_MAX256
> #define BTRFS_BACKREF_REV_SHIFT 56
> --
> 2.19.1
>
[PATCH 1/9] btrfs: switch BTRFS_FS_STATE_* to enums
We can use simple enum for values that are not part of on-disk format:
global filesystem states.
Signed-off-by: David Sterba
---
fs/btrfs/ctree.h | 25 +++--
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index a98507fa9192..f82ec5e41b0c 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -109,13 +109,26 @@ static inline unsigned long btrfs_chunk_item_size(int
num_stripes)
}
/*
- * File system states
+ * Runtime (in-memory) states of filesystem
*/
-#define BTRFS_FS_STATE_ERROR 0
-#define BTRFS_FS_STATE_REMOUNTING 1
-#define BTRFS_FS_STATE_TRANS_ABORTED 2
-#define BTRFS_FS_STATE_DEV_REPLACING 3
-#define BTRFS_FS_STATE_DUMMY_FS_INFO 4
+enum {
+ /* Global indicator of serious filesysystem errors */
+ BTRFS_FS_STATE_ERROR,
+ /*
+* Filesystem is being remounted, allow to skip some operations, like
+* defrag
+*/
+ BTRFS_FS_STATE_REMOUNTING,
+ /* Track if the transaction abort has been reported */
+ BTRFS_FS_STATE_TRANS_ABORTED,
+ /*
+* Indicate that replace source or target device state is changed and
+* allow to block bio operations
+*/
+ BTRFS_FS_STATE_DEV_REPLACING,
+ /* The btrfs_fs_info created for self-tests */
+ BTRFS_FS_STATE_DUMMY_FS_INFO,
+};
#define BTRFS_BACKREF_REV_MAX 256
#define BTRFS_BACKREF_REV_SHIFT56
--
2.19.1
