2009-09-17 13:25:24

by Theodore Ts'o

[permalink] [raw]
Subject: [PATCH] ext4: store EXT4_EXT_MIGRATE in i_state instead of i_flags

EXT4_EXT_MIGRATE is only intended to be used for an in-memory flag,
and the hex value assigned to it collides with FS_DIRECTIO_FL (which
is also stored in i_flags). There's no reason for the
EXT4_EXT_MIGRATE bit to be stored in i_flags, so we switch it to use
i_state instead.

Cc: "Aneesh Kumar K.V" <[email protected]>
Signed-off-by: "Theodore Ts'o" <[email protected]>
---
fs/ext4/ext4.h | 2 +-
fs/ext4/inode.c | 6 ++----
fs/ext4/migrate.c | 20 ++++++++++----------
3 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index b558de8..98cf2b3 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -268,7 +268,6 @@ struct flex_groups {
#define EXT4_TOPDIR_FL 0x00020000 /* Top of directory hierarchies*/
#define EXT4_HUGE_FILE_FL 0x00040000 /* Set to each huge file */
#define EXT4_EXTENTS_FL 0x00080000 /* Inode uses extents */
-#define EXT4_EXT_MIGRATE 0x00100000 /* Inode is migrating */
#define EXT4_RESERVED_FL 0x80000000 /* reserved for ext4 lib */

#define EXT4_FL_USER_VISIBLE 0x000BDFFF /* User visible flags */
@@ -306,6 +305,7 @@ static inline __u32 ext4_mask_flags(umode_t mode, __u32 flags)
#define EXT4_STATE_XATTR 0x00000004 /* has in-inode xattrs */
#define EXT4_STATE_NO_EXPAND 0x00000008 /* No space for expansion */
#define EXT4_STATE_DA_ALLOC_CLOSE 0x00000010 /* Alloc DA blks on close */
+#define EXT4_STATE_EXT_MIGRATE 0x00000020 /* Inode is migrating */

/* Used to pass group descriptor data when online resize is done */
struct ext4_new_group_input {
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 5a89792..a5b4ce4 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1255,8 +1255,7 @@ int ext4_get_blocks(handle_t *handle, struct inode *inode, sector_t block,
* i_data's format changing. Force the migrate
* to fail by clearing migrate flags
*/
- EXT4_I(inode)->i_flags = EXT4_I(inode)->i_flags &
- ~EXT4_EXT_MIGRATE;
+ EXT4_I(inode)->i_state &= ~EXT4_STATE_EXT_MIGRATE;
}
}

@@ -4596,8 +4595,7 @@ static int ext4_do_update_inode(handle_t *handle,
if (ext4_inode_blocks_set(handle, raw_inode, ei))
goto out_brelse;
raw_inode->i_dtime = cpu_to_le32(ei->i_dtime);
- /* clear the migrate flag in the raw_inode */
- raw_inode->i_flags = cpu_to_le32(ei->i_flags & ~EXT4_EXT_MIGRATE);
+ raw_inode->i_flags = cpu_to_le32(ei->i_flags);
if (EXT4_SB(inode->i_sb)->s_es->s_creator_os !=
cpu_to_le32(EXT4_OS_HURD))
raw_inode->i_file_acl_high =
diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
index 05361ad..bf519f2 100644
--- a/fs/ext4/migrate.c
+++ b/fs/ext4/migrate.c
@@ -353,17 +353,16 @@ static int ext4_ext_swap_inode_data(handle_t *handle, struct inode *inode,

down_write(&EXT4_I(inode)->i_data_sem);
/*
- * if EXT4_EXT_MIGRATE is cleared a block allocation
+ * if EXT4_STATE_EXT_MIGRATE is cleared a block allocation
* happened after we started the migrate. We need to
* fail the migrate
*/
- if (!(EXT4_I(inode)->i_flags & EXT4_EXT_MIGRATE)) {
+ if (!(EXT4_I(inode)->i_state & EXT4_STATE_EXT_MIGRATE)) {
retval = -EAGAIN;
up_write(&EXT4_I(inode)->i_data_sem);
goto err_out;
} else
- EXT4_I(inode)->i_flags = EXT4_I(inode)->i_flags &
- ~EXT4_EXT_MIGRATE;
+ EXT4_I(inode)->i_state &= ~EXT4_STATE_EXT_MIGRATE;
/*
* We have the extent map build with the tmp inode.
* Now copy the i_data across
@@ -517,14 +516,15 @@ int ext4_ext_migrate(struct inode *inode)
* when we add extents we extent the journal
*/
/*
- * Even though we take i_mutex we can still cause block allocation
- * via mmap write to holes. If we have allocated new blocks we fail
- * migrate. New block allocation will clear EXT4_EXT_MIGRATE flag.
- * The flag is updated with i_data_sem held to prevent racing with
- * block allocation.
+ * Even though we take i_mutex we can still cause block
+ * allocation via mmap write to holes. If we have allocated
+ * new blocks we fail migrate. New block allocation will
+ * clear EXT4_STATE_EXT_MIGRATE flag. The flag is updated
+ * with i_data_sem held to prevent racing with block
+ * allocation.
*/
down_read((&EXT4_I(inode)->i_data_sem));
- EXT4_I(inode)->i_flags = EXT4_I(inode)->i_flags | EXT4_EXT_MIGRATE;
+ EXT4_I(inode)->i_state |= EXT4_STATE_EXT_MIGRATE;
up_read((&EXT4_I(inode)->i_data_sem));

handle = ext4_journal_start(inode, 1);
--
1.6.3.2.1.gb9f7d.dirty



2009-09-17 17:04:24

by Andreas Dilger

[permalink] [raw]
Subject: Re: [PATCH] ext4: store EXT4_EXT_MIGRATE in i_state instead of i_flags

On Sep 17, 2009 09:25 -0400, Theodore Ts'o wrote:
> EXT4_EXT_MIGRATE is only intended to be used for an in-memory flag,
> and the hex value assigned to it collides with FS_DIRECTIO_FL (which
> is also stored in i_flags). There's no reason for the
> EXT4_EXT_MIGRATE bit to be stored in i_flags, so we switch it to use
> i_state instead.
>
> Cc: "Aneesh Kumar K.V" <[email protected]>
> Signed-off-by: "Theodore Ts'o" <[email protected]>

Reviewed-by: Andreas Dilger <[email protected]>

> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index b558de8..98cf2b3 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -268,7 +268,6 @@ struct flex_groups {
> #define EXT4_TOPDIR_FL 0x00020000 /* Top of directory hierarchies*/
> #define EXT4_HUGE_FILE_FL 0x00040000 /* Set to each huge file */
> #define EXT4_EXTENTS_FL 0x00080000 /* Inode uses extents */
> -#define EXT4_EXT_MIGRATE 0x00100000 /* Inode is migrating */

You might consider adding in an "EXT4_DIRECTIO_FL" here as a placeholder,
to ensure it is not used for some other conflicting purpose.

> #define EXT4_RESERVED_FL 0x80000000 /* reserved for ext4 lib */
>
> #define EXT4_FL_USER_VISIBLE 0x000BDFFF /* User visible flags */
> @@ -306,6 +305,7 @@ static inline __u32 ext4_mask_flags(umode_t mode, __u32 flags)
> #define EXT4_STATE_XATTR 0x00000004 /* has in-inode xattrs */
> #define EXT4_STATE_NO_EXPAND 0x00000008 /* No space for expansion */
> #define EXT4_STATE_DA_ALLOC_CLOSE 0x00000010 /* Alloc DA blks on close */
> +#define EXT4_STATE_EXT_MIGRATE 0x00000020 /* Inode is migrating */
>
> /* Used to pass group descriptor data when online resize is done */
> struct ext4_new_group_input {
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 5a89792..a5b4ce4 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1255,8 +1255,7 @@ int ext4_get_blocks(handle_t *handle, struct inode *inode, sector_t block,
> * i_data's format changing. Force the migrate
> * to fail by clearing migrate flags
> */
> - EXT4_I(inode)->i_flags = EXT4_I(inode)->i_flags &
> - ~EXT4_EXT_MIGRATE;
> + EXT4_I(inode)->i_state &= ~EXT4_STATE_EXT_MIGRATE;
> }
> }
>
> @@ -4596,8 +4595,7 @@ static int ext4_do_update_inode(handle_t *handle,
> if (ext4_inode_blocks_set(handle, raw_inode, ei))
> goto out_brelse;
> raw_inode->i_dtime = cpu_to_le32(ei->i_dtime);
> - /* clear the migrate flag in the raw_inode */
> - raw_inode->i_flags = cpu_to_le32(ei->i_flags & ~EXT4_EXT_MIGRATE);
> + raw_inode->i_flags = cpu_to_le32(ei->i_flags);
> if (EXT4_SB(inode->i_sb)->s_es->s_creator_os !=
> cpu_to_le32(EXT4_OS_HURD))
> raw_inode->i_file_acl_high =
> diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
> index 05361ad..bf519f2 100644
> --- a/fs/ext4/migrate.c
> +++ b/fs/ext4/migrate.c
> @@ -353,17 +353,16 @@ static int ext4_ext_swap_inode_data(handle_t *handle, struct inode *inode,
>
> down_write(&EXT4_I(inode)->i_data_sem);
> /*
> - * if EXT4_EXT_MIGRATE is cleared a block allocation
> + * if EXT4_STATE_EXT_MIGRATE is cleared a block allocation
> * happened after we started the migrate. We need to
> * fail the migrate
> */
> - if (!(EXT4_I(inode)->i_flags & EXT4_EXT_MIGRATE)) {
> + if (!(EXT4_I(inode)->i_state & EXT4_STATE_EXT_MIGRATE)) {
> retval = -EAGAIN;
> up_write(&EXT4_I(inode)->i_data_sem);
> goto err_out;
> } else
> - EXT4_I(inode)->i_flags = EXT4_I(inode)->i_flags &
> - ~EXT4_EXT_MIGRATE;
> + EXT4_I(inode)->i_state &= ~EXT4_STATE_EXT_MIGRATE;
> /*
> * We have the extent map build with the tmp inode.
> * Now copy the i_data across
> @@ -517,14 +516,15 @@ int ext4_ext_migrate(struct inode *inode)
> * when we add extents we extent the journal
> */
> /*
> - * Even though we take i_mutex we can still cause block allocation
> - * via mmap write to holes. If we have allocated new blocks we fail
> - * migrate. New block allocation will clear EXT4_EXT_MIGRATE flag.
> - * The flag is updated with i_data_sem held to prevent racing with
> - * block allocation.
> + * Even though we take i_mutex we can still cause block
> + * allocation via mmap write to holes. If we have allocated
> + * new blocks we fail migrate. New block allocation will
> + * clear EXT4_STATE_EXT_MIGRATE flag. The flag is updated
> + * with i_data_sem held to prevent racing with block
> + * allocation.
> */
> down_read((&EXT4_I(inode)->i_data_sem));
> - EXT4_I(inode)->i_flags = EXT4_I(inode)->i_flags | EXT4_EXT_MIGRATE;
> + EXT4_I(inode)->i_state |= EXT4_STATE_EXT_MIGRATE;
> up_read((&EXT4_I(inode)->i_data_sem));
>
> handle = ext4_journal_start(inode, 1);
> --
> 1.6.3.2.1.gb9f7d.dirty
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.