| Message ID | bd1d34e39a5fe9d226da167f8b970527f980846d.1660735025.git.fdmanana@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: some updates to delayed items and inode logging | expand |
On Wed, Aug 17, 2022 at 12:22:42PM +0100, fdmanana@kernel.org wrote: > } > > item->index = index; > - item->ins_or_del = BTRFS_DELAYED_DELETION_ITEM; > > ret = btrfs_delayed_item_reserve_metadata(trans, item); > /* > diff --git a/fs/btrfs/delayed-inode.h b/fs/btrfs/delayed-inode.h > index fd6fe785f748..729d352ca8a1 100644 > --- a/fs/btrfs/delayed-inode.h > +++ b/fs/btrfs/delayed-inode.h > @@ -16,9 +16,10 @@ > #include <linux/refcount.h> > #include "ctree.h" > > -/* types of the delayed item */ > -#define BTRFS_DELAYED_INSERTION_ITEM 1 > -#define BTRFS_DELAYED_DELETION_ITEM 2 > +enum btrfs_delayed_item_type { > + BTRFS_DELAYED_INSERTION_ITEM, > + BTRFS_DELAYED_DELETION_ITEM > +}; > > struct btrfs_delayed_root { > spinlock_t lock; > @@ -80,8 +81,9 @@ struct btrfs_delayed_item { > u64 bytes_reserved; > struct btrfs_delayed_node *delayed_node; > refcount_t refs; > - int ins_or_del; > - u32 data_len; > + enum btrfs_delayed_item_type type:1; Bit fields next to atomicly accessed variables could be problemantic on architectures without safe unaligned access. Either this can be :8 or moved after 'key' where's a 7 byte hole.
On Mon, Aug 22, 2022 at 03:43:26PM +0200, David Sterba wrote: > On Wed, Aug 17, 2022 at 12:22:42PM +0100, fdmanana@kernel.org wrote: > > } > > > > item->index = index; > > - item->ins_or_del = BTRFS_DELAYED_DELETION_ITEM; > > > > ret = btrfs_delayed_item_reserve_metadata(trans, item); > > /* > > diff --git a/fs/btrfs/delayed-inode.h b/fs/btrfs/delayed-inode.h > > index fd6fe785f748..729d352ca8a1 100644 > > --- a/fs/btrfs/delayed-inode.h > > +++ b/fs/btrfs/delayed-inode.h > > @@ -16,9 +16,10 @@ > > #include <linux/refcount.h> > > #include "ctree.h" > > > > -/* types of the delayed item */ > > -#define BTRFS_DELAYED_INSERTION_ITEM 1 > > -#define BTRFS_DELAYED_DELETION_ITEM 2 > > +enum btrfs_delayed_item_type { > > + BTRFS_DELAYED_INSERTION_ITEM, > > + BTRFS_DELAYED_DELETION_ITEM > > +}; > > > > struct btrfs_delayed_root { > > spinlock_t lock; > > @@ -80,8 +81,9 @@ struct btrfs_delayed_item { > > u64 bytes_reserved; > > struct btrfs_delayed_node *delayed_node; > > refcount_t refs; > > - int ins_or_del; > > - u32 data_len; > > + enum btrfs_delayed_item_type type:1; > > Bit fields next to atomicly accessed variables could be problemantic on > architectures without safe unaligned access. Either this can be :8 or > moved after 'key' where's a 7 byte hole. Yep, I forgot that. The 'key' doesn't exist anymore, it was removed in a previous patch of the series, so the 7 bytes holes is not there anymore. I don't see any other place in the structure where I can move the fields and still reduce its size. If switching from :1 to :8 is enough to guarantee safety, I guess it's the only solution. Do you me to send a new version just to switch :1 to :8 or will you do that change? Thanks.
On Mon, Aug 22, 2022 at 03:15:32PM +0100, Filipe Manana wrote: > On Mon, Aug 22, 2022 at 03:43:26PM +0200, David Sterba wrote: > > On Wed, Aug 17, 2022 at 12:22:42PM +0100, fdmanana@kernel.org wrote: > > > } > > > > > > item->index = index; > > > - item->ins_or_del = BTRFS_DELAYED_DELETION_ITEM; > > > > > > ret = btrfs_delayed_item_reserve_metadata(trans, item); > > > /* > > > diff --git a/fs/btrfs/delayed-inode.h b/fs/btrfs/delayed-inode.h > > > index fd6fe785f748..729d352ca8a1 100644 > > > --- a/fs/btrfs/delayed-inode.h > > > +++ b/fs/btrfs/delayed-inode.h > > > @@ -16,9 +16,10 @@ > > > #include <linux/refcount.h> > > > #include "ctree.h" > > > > > > -/* types of the delayed item */ > > > -#define BTRFS_DELAYED_INSERTION_ITEM 1 > > > -#define BTRFS_DELAYED_DELETION_ITEM 2 > > > +enum btrfs_delayed_item_type { > > > + BTRFS_DELAYED_INSERTION_ITEM, > > > + BTRFS_DELAYED_DELETION_ITEM > > > +}; > > > > > > struct btrfs_delayed_root { > > > spinlock_t lock; > > > @@ -80,8 +81,9 @@ struct btrfs_delayed_item { > > > u64 bytes_reserved; > > > struct btrfs_delayed_node *delayed_node; > > > refcount_t refs; > > > - int ins_or_del; > > > - u32 data_len; > > > + enum btrfs_delayed_item_type type:1; > > > > Bit fields next to atomicly accessed variables could be problemantic on > > architectures without safe unaligned access. Either this can be :8 or > > moved after 'key' where's a 7 byte hole. > > Yep, I forgot that. > > The 'key' doesn't exist anymore, it was removed in a previous patch of > the series, so the 7 bytes holes is not there anymore. Right, I did not check on the exact commit first. > I don't see any other place in the structure where I can move the fields > and still reduce its size. If switching from :1 to :8 is enough to > guarantee safety, I guess it's the only solution. Yeah that's find and there's only one byte unused, leaving the char data[] aligned to 8 bytes: struct btrfs_delayed_item { struct rb_node rb_node __attribute__((__aligned__(8))); /* 0 24 */ u64 index; /* 24 8 */ struct list_head tree_list; /* 32 16 */ struct list_head readdir_list; /* 48 16 */ /* --- cacheline 1 boundary (64 bytes) --- */ u64 bytes_reserved; /* 64 8 */ struct btrfs_delayed_node * delayed_node; /* 72 8 */ refcount_t refs; /* 80 4 */ enum btrfs_delayed_item_type type:8; /* 84: 0 4 */ /* XXX 8 bits hole, try to pack */ /* Bitfield combined with next fields */ u16 data_len; /* 86 2 */ char data[]; /* 88 0 */ /* size: 88, cachelines: 2, members: 10 */ /* sum members: 86 */ /* sum bitfield members: 8 bits, bit holes: 1, sum bit holes: 8 bits */ /* forced alignments: 1 */ /* last cacheline: 24 bytes */ } __attribute__((__aligned__(8))); > Do you me to send a new version just to switch :1 to :8 or will you do > that change? I'll do that, it's a simple change but I wanted to let you know about that.
On Mon, Aug 22, 2022 at 05:29:22PM +0200, David Sterba wrote: > On Mon, Aug 22, 2022 at 03:15:32PM +0100, Filipe Manana wrote: > > On Mon, Aug 22, 2022 at 03:43:26PM +0200, David Sterba wrote: > > > On Wed, Aug 17, 2022 at 12:22:42PM +0100, fdmanana@kernel.org wrote: > > > > } > > > > > > > > item->index = index; > > > > - item->ins_or_del = BTRFS_DELAYED_DELETION_ITEM; > > > > > > > > ret = btrfs_delayed_item_reserve_metadata(trans, item); > > > > /* > > > > diff --git a/fs/btrfs/delayed-inode.h b/fs/btrfs/delayed-inode.h > > > > index fd6fe785f748..729d352ca8a1 100644 > > > > --- a/fs/btrfs/delayed-inode.h > > > > +++ b/fs/btrfs/delayed-inode.h > > > > @@ -16,9 +16,10 @@ > > > > #include <linux/refcount.h> > > > > #include "ctree.h" > > > > > > > > -/* types of the delayed item */ > > > > -#define BTRFS_DELAYED_INSERTION_ITEM 1 > > > > -#define BTRFS_DELAYED_DELETION_ITEM 2 > > > > +enum btrfs_delayed_item_type { > > > > + BTRFS_DELAYED_INSERTION_ITEM, > > > > + BTRFS_DELAYED_DELETION_ITEM > > > > +}; > > > > > > > > struct btrfs_delayed_root { > > > > spinlock_t lock; > > > > @@ -80,8 +81,9 @@ struct btrfs_delayed_item { > > > > u64 bytes_reserved; > > > > struct btrfs_delayed_node *delayed_node; > > > > refcount_t refs; > > > > - int ins_or_del; > > > > - u32 data_len; > > > > + enum btrfs_delayed_item_type type:1; > > > > > > Bit fields next to atomicly accessed variables could be problemantic on > > > architectures without safe unaligned access. Either this can be :8 or > > > moved after 'key' where's a 7 byte hole. > > > > Yep, I forgot that. > > > > The 'key' doesn't exist anymore, it was removed in a previous patch of > > the series, so the 7 bytes holes is not there anymore. > > Right, I did not check on the exact commit first. > > > I don't see any other place in the structure where I can move the fields > > and still reduce its size. If switching from :1 to :8 is enough to > > guarantee safety, I guess it's the only solution. > > Yeah that's find and there's only one byte unused, leaving the char > data[] aligned to 8 bytes: > > struct btrfs_delayed_item { > struct rb_node rb_node __attribute__((__aligned__(8))); /* 0 24 */ > u64 index; /* 24 8 */ > struct list_head tree_list; /* 32 16 */ > struct list_head readdir_list; /* 48 16 */ > /* --- cacheline 1 boundary (64 bytes) --- */ > u64 bytes_reserved; /* 64 8 */ > struct btrfs_delayed_node * delayed_node; /* 72 8 */ > refcount_t refs; /* 80 4 */ > enum btrfs_delayed_item_type type:8; /* 84: 0 4 */ > > /* XXX 8 bits hole, try to pack */ > /* Bitfield combined with next fields */ > > u16 data_len; /* 86 2 */ > char data[]; /* 88 0 */ > > /* size: 88, cachelines: 2, members: 10 */ > /* sum members: 86 */ > /* sum bitfield members: 8 bits, bit holes: 1, sum bit holes: 8 bits */ > /* forced alignments: 1 */ > /* last cacheline: 24 bytes */ > } __attribute__((__aligned__(8))); > > > Do you me to send a new version just to switch :1 to :8 or will you do > > that change? > > I'll do that, it's a simple change but I wanted to let you know about > that. Ok, thanks for that, it's easy to miss. I have some vague memory from that causing problems in the past on Alpha IIRC.
diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c index a8947ac00681..b35eddcac846 100644 --- a/fs/btrfs/delayed-inode.c +++ b/fs/btrfs/delayed-inode.c @@ -302,14 +302,16 @@ static inline void btrfs_release_prepared_delayed_node( __btrfs_release_delayed_node(node, 1); } -static struct btrfs_delayed_item *btrfs_alloc_delayed_item(u32 data_len, - struct btrfs_delayed_node *node) +static struct btrfs_delayed_item *btrfs_alloc_delayed_item(u16 data_len, + struct btrfs_delayed_node *node, + enum btrfs_delayed_item_type type) { struct btrfs_delayed_item *item; + item = kmalloc(sizeof(*item) + data_len, GFP_NOFS); if (item) { item->data_len = data_len; - item->ins_or_del = 0; + item->type = type; item->bytes_reserved = 0; item->delayed_node = node; RB_CLEAR_NODE(&item->rb_node); @@ -356,12 +358,11 @@ static int __btrfs_add_delayed_item(struct btrfs_delayed_node *delayed_node, struct btrfs_delayed_item *item; bool leftmost = true; - if (ins->ins_or_del == BTRFS_DELAYED_INSERTION_ITEM) + if (ins->type == BTRFS_DELAYED_INSERTION_ITEM) root = &delayed_node->ins_root; - else if (ins->ins_or_del == BTRFS_DELAYED_DELETION_ITEM) - root = &delayed_node->del_root; else - BUG(); + root = &delayed_node->del_root; + p = &root->rb_root.rb_node; node = &ins->rb_node; @@ -383,7 +384,7 @@ static int __btrfs_add_delayed_item(struct btrfs_delayed_node *delayed_node, rb_link_node(node, parent_node, p); rb_insert_color_cached(node, root, leftmost); - if (ins->ins_or_del == BTRFS_DELAYED_INSERTION_ITEM && + if (ins->type == BTRFS_DELAYED_INSERTION_ITEM && ins->index >= delayed_node->index_cnt) delayed_node->index_cnt = ins->index + 1; @@ -414,10 +415,8 @@ static void __btrfs_remove_delayed_item(struct btrfs_delayed_item *delayed_item) delayed_root = delayed_item->delayed_node->root->fs_info->delayed_root; BUG_ON(!delayed_root); - BUG_ON(delayed_item->ins_or_del != BTRFS_DELAYED_DELETION_ITEM && - delayed_item->ins_or_del != BTRFS_DELAYED_INSERTION_ITEM); - if (delayed_item->ins_or_del == BTRFS_DELAYED_INSERTION_ITEM) + if (delayed_item->type == BTRFS_DELAYED_INSERTION_ITEM) root = &delayed_item->delayed_node->ins_root; else root = &delayed_item->delayed_node->del_root; @@ -509,7 +508,7 @@ static int btrfs_delayed_item_reserve_metadata(struct btrfs_trans_handle *trans, * for the number of leaves that will be used, based on the delayed * node's index_items_size field. */ - if (item->ins_or_del == BTRFS_DELAYED_DELETION_ITEM) + if (item->type == BTRFS_DELAYED_DELETION_ITEM) item->bytes_reserved = num_bytes; } @@ -646,6 +645,7 @@ static int btrfs_insert_delayed_item(struct btrfs_trans_handle *trans, const int max_size = BTRFS_LEAF_DATA_SIZE(fs_info); struct btrfs_item_batch batch; struct btrfs_key first_key; + const u32 first_data_size = first_item->data_len; int total_size; char *ins_data = NULL; int ret; @@ -674,9 +674,9 @@ static int btrfs_insert_delayed_item(struct btrfs_trans_handle *trans, ASSERT(first_item->bytes_reserved == 0); list_add_tail(&first_item->tree_list, &item_list); - batch.total_data_size = first_item->data_len; + batch.total_data_size = first_data_size; batch.nr = 1; - total_size = first_item->data_len + sizeof(struct btrfs_item); + total_size = first_data_size + sizeof(struct btrfs_item); curr = first_item; while (true) { @@ -711,7 +711,7 @@ static int btrfs_insert_delayed_item(struct btrfs_trans_handle *trans, first_key.type = BTRFS_DIR_INDEX_KEY; first_key.offset = first_item->index; batch.keys = &first_key; - batch.data_sizes = &first_item->data_len; + batch.data_sizes = &first_data_size; } else { struct btrfs_key *ins_keys; u32 *ins_sizes; @@ -1427,14 +1427,14 @@ int btrfs_insert_delayed_dir_index(struct btrfs_trans_handle *trans, return PTR_ERR(delayed_node); delayed_item = btrfs_alloc_delayed_item(sizeof(*dir_item) + name_len, - delayed_node); + delayed_node, + BTRFS_DELAYED_INSERTION_ITEM); if (!delayed_item) { ret = -ENOMEM; goto release_node; } delayed_item->index = index; - delayed_item->ins_or_del = BTRFS_DELAYED_INSERTION_ITEM; dir_item = (struct btrfs_dir_item *)delayed_item->data; dir_item->location = *disk_key; @@ -1566,14 +1566,13 @@ int btrfs_delete_delayed_dir_index(struct btrfs_trans_handle *trans, if (!ret) goto end; - item = btrfs_alloc_delayed_item(0, node); + item = btrfs_alloc_delayed_item(0, node, BTRFS_DELAYED_DELETION_ITEM); if (!item) { ret = -ENOMEM; goto end; } item->index = index; - item->ins_or_del = BTRFS_DELAYED_DELETION_ITEM; ret = btrfs_delayed_item_reserve_metadata(trans, item); /* diff --git a/fs/btrfs/delayed-inode.h b/fs/btrfs/delayed-inode.h index fd6fe785f748..729d352ca8a1 100644 --- a/fs/btrfs/delayed-inode.h +++ b/fs/btrfs/delayed-inode.h @@ -16,9 +16,10 @@ #include <linux/refcount.h> #include "ctree.h" -/* types of the delayed item */ -#define BTRFS_DELAYED_INSERTION_ITEM 1 -#define BTRFS_DELAYED_DELETION_ITEM 2 +enum btrfs_delayed_item_type { + BTRFS_DELAYED_INSERTION_ITEM, + BTRFS_DELAYED_DELETION_ITEM +}; struct btrfs_delayed_root { spinlock_t lock; @@ -80,8 +81,9 @@ struct btrfs_delayed_item { u64 bytes_reserved; struct btrfs_delayed_node *delayed_node; refcount_t refs; - int ins_or_del; - u32 data_len; + enum btrfs_delayed_item_type type:1; + /* The maximum leaf size is 64K, so u16 is more than enough. */ + u16 data_len; char data[]; };