| Message ID | 4365d4183bf65240273c8602faa0a75e88906d38.1543348078.git.dsterba@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Switch defines to enums | expand |
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 <osandov@fb.com> Some typos/wording suggestions below. > Signed-off-by: David Sterba <dsterba@suse.com> > --- > 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"? > + 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_MAX 256 > #define BTRFS_BACKREF_REV_SHIFT 56 > -- > 2.19.1 >
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 <dsterba@suse.com> Good comment. Reviewed-by: Qu Wenruo <wqu@suse.com> 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_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_SHIFT 56 >
Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
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 <osandov@fb.com> > > Some typos/wording suggestions below. > > > Signed-off-by: David Sterba <dsterba@suse.com> > > --- > > 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.
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_SHIFT 56
We can use simple enum for values that are not part of on-disk format: global filesystem states. Signed-off-by: David Sterba <dsterba@suse.com> --- fs/btrfs/ctree.h | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-)