* [PATCH 0/2] Reserve more space for property inheritance @ 2019-02-07 16:54 Josef Bacik 2019-02-07 16:54 ` [PATCH 1/2] btrfs: reserve space for inheriting properties Josef Bacik 2019-02-07 16:54 ` [PATCH 2/2] btrfs: use the existing credit for our first prop Josef Bacik 0 siblings, 2 replies; 8+ messages in thread From: Josef Bacik @ 2019-02-07 16:54 UTC (permalink / raw) To: linux-btrfs, kernel-team We've been seeing messages on our build servers where we fail to reserve space for the compression property and thus fail to create the property at inode creation time. This is because we do not pre-reserve the space, instead opting to use NO_FLUSH which can fail transiently, and create these messages. Instead just pre-reserve an extra items worth of space, and use that space when we inherit our property. We only have the compression property right now, so we can get away with this, and if we add any more properties we have the backup method of doing the NO_FLUSH reservation. However in the future we may want to take a more nuanced approach to property space reservation if we add a bunch of new properties. Thanks, Josef ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] btrfs: reserve space for inheriting properties 2019-02-07 16:54 [PATCH 0/2] Reserve more space for property inheritance Josef Bacik @ 2019-02-07 16:54 ` Josef Bacik 2019-02-08 6:30 ` Nikolay Borisov 2019-02-13 12:37 ` Filipe Manana 2019-02-07 16:54 ` [PATCH 2/2] btrfs: use the existing credit for our first prop Josef Bacik 1 sibling, 2 replies; 8+ messages in thread From: Josef Bacik @ 2019-02-07 16:54 UTC (permalink / raw) To: linux-btrfs, kernel-team We've been seeing errors on our build servers related to failing to inherit inode properties. This is because we do not pre-reserve space for them, instead trying to reserve space with NO_FLUSH at inheritance time. NO_FLUSH can transiently fail, but we'll still complain. It's just an extra credit, so simply add that to the places that call btrfs_new_inode and call it good enough. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/inode.c | 78 ++++++++++++++++++++++---------------------------------- fs/btrfs/ioctl.c | 27 ++++++++++++-------- 2 files changed, 46 insertions(+), 59 deletions(-) diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 6126de9b8b9c..0da4a9d6d9fe 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -59,6 +59,14 @@ struct btrfs_dio_data { int overwrite; }; +/* + * 2 for inode item and ref + * 2 for dir items + * 1 for xattr if selinux is on + * 1 for inherited properties + */ +#define BTRFS_NEW_INODE_ITEMS 6 + static const struct inode_operations btrfs_dir_inode_operations; static const struct inode_operations btrfs_symlink_inode_operations; static const struct inode_operations btrfs_dir_ro_inode_operations; @@ -6479,12 +6487,7 @@ static int btrfs_mknod(struct inode *dir, struct dentry *dentry, u64 objectid; u64 index = 0; - /* - * 2 for inode item and ref - * 2 for dir items - * 1 for xattr if selinux is on - */ - trans = btrfs_start_transaction(root, 5); + trans = btrfs_start_transaction(root, BTRFS_NEW_INODE_ITEMS); if (IS_ERR(trans)) return PTR_ERR(trans); @@ -6543,12 +6546,7 @@ static int btrfs_create(struct inode *dir, struct dentry *dentry, u64 objectid; u64 index = 0; - /* - * 2 for inode item and ref - * 2 for dir items - * 1 for xattr if selinux is on - */ - trans = btrfs_start_transaction(root, 5); + trans = btrfs_start_transaction(root, BTRFS_NEW_INODE_ITEMS); if (IS_ERR(trans)) return PTR_ERR(trans); @@ -6695,12 +6693,7 @@ static int btrfs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode) u64 objectid = 0; u64 index = 0; - /* - * 2 items for inode and ref - * 2 items for dir items - * 1 for xattr if selinux is on - */ - trans = btrfs_start_transaction(root, 5); + trans = btrfs_start_transaction(root, BTRFS_NEW_INODE_ITEMS); if (IS_ERR(trans)) return PTR_ERR(trans); @@ -9428,14 +9421,11 @@ static int btrfs_rename_exchange(struct inode *old_dir, down_read(&fs_info->subvol_sem); /* - * We want to reserve the absolute worst case amount of items. So if - * both inodes are subvols and we need to unlink them then that would - * require 4 item modifications, but if they are both normal inodes it - * would require 5 item modifications, so we'll assume their normal - * inodes. So 5 * 2 is 10, plus 2 for the new links, so 12 total items - * should cover the worst case number of items we'll modify. + * The same math from btrfs_rename applies here, except we need an extra + * 2 items for the new links. */ - trans = btrfs_start_transaction(root, 12); + trans = btrfs_start_transaction(root, + (BTRFS_NEW_INODE_ITEMS << 1) + 2); if (IS_ERR(trans)) { ret = PTR_ERR(trans); goto out_notrans; @@ -9768,19 +9758,19 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry, if (old_ino == BTRFS_FIRST_FREE_OBJECTID) down_read(&fs_info->subvol_sem); /* - * We want to reserve the absolute worst case amount of items. So if - * both inodes are subvols and we need to unlink them then that would - * require 4 item modifications, but if they are both normal inodes it - * would require 5 item modifications, so we'll assume they are normal - * inodes. So 5 * 2 is 10, plus 1 for the new link, so 11 total items - * should cover the worst case number of items we'll modify. - * If our rename has the whiteout flag, we need more 5 units for the - * new inode (1 inode item, 1 inode ref, 2 dir items and 1 xattr item - * when selinux is enabled). + * We want to reserve the absolute worst case amount of items. Subvol + * inodes don't have an inode item to worry about and don't have a + * selinux attr, so we use the BTRFS_NEW_INODE_ITEMS counter for how + * much it costs per inode to modify. Worse case we'll have to mess + * with 2 inodes, so 2 x BTRFS_NEW_INODE_ITEMS, and then we need an + * extra reservation for the new link. + * + * If our rename has the whiteout flag we need a full new inode which + * means another set of BTRFS_NEW_INODE_ITEMS. */ - trans_num_items = 11; + trans_num_items = (BTRFS_NEW_INODE_ITEMS << 1) + 1; if (flags & RENAME_WHITEOUT) - trans_num_items += 5; + trans_num_items += BTRFS_NEW_INODE_ITEMS; trans = btrfs_start_transaction(root, trans_num_items); if (IS_ERR(trans)) { ret = PTR_ERR(trans); @@ -10149,14 +10139,8 @@ static int btrfs_symlink(struct inode *dir, struct dentry *dentry, if (name_len > BTRFS_MAX_INLINE_DATA_SIZE(fs_info)) return -ENAMETOOLONG; - /* - * 2 items for inode item and ref - * 2 items for dir items - * 1 item for updating parent inode item - * 1 item for the inline extent item - * 1 item for xattr if selinux is on - */ - trans = btrfs_start_transaction(root, 7); + /* 1 item for the inline extent item */ + trans = btrfs_start_transaction(root, BTRFS_NEW_INODE_ITEMS + 1); if (IS_ERR(trans)) return PTR_ERR(trans); @@ -10427,10 +10411,8 @@ static int btrfs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode) u64 index; int ret = 0; - /* - * 5 units required for adding orphan entry - */ - trans = btrfs_start_transaction(root, 5); + /* 1 unit required for adding orphan entry */ + trans = btrfs_start_transaction(root, BTRFS_NEW_INODE_ITEMS + 1); if (IS_ERR(trans)) return PTR_ERR(trans); diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index f38a659c918c..21f8ab2d8570 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -83,6 +83,17 @@ struct btrfs_ioctl_send_args_32 { struct btrfs_ioctl_send_args_32) #endif +/* + * 1 - parent dir inode + * 2 - dir entries + * 1 - root item + * 2 - root ref/backref + * 1 - root of snapshot + * 1 - UUID item + * 1 - properties + */ +#define BTRFS_NEW_ROOT_ITEMS 9 + static int btrfs_clone(struct inode *src, struct inode *inode, u64 off, u64 olen, u64 olen_aligned, u64 destoff, int no_time_update); @@ -596,7 +607,8 @@ static noinline int create_subvol(struct inode *dir, * The same as the snapshot creation, please see the comment * of create_snapshot(). */ - ret = btrfs_subvolume_reserve_metadata(root, &block_rsv, 8, false); + ret = btrfs_subvolume_reserve_metadata(root, &block_rsv, + BTRFS_NEW_ROOT_ITEMS, false); if (ret) goto fail_free; @@ -804,17 +816,10 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir, btrfs_init_block_rsv(&pending_snapshot->block_rsv, BTRFS_BLOCK_RSV_TEMP); - /* - * 1 - parent dir inode - * 2 - dir entries - * 1 - root item - * 2 - root ref/backref - * 1 - root of snapshot - * 1 - UUID item - */ + ret = btrfs_subvolume_reserve_metadata(BTRFS_I(dir)->root, - &pending_snapshot->block_rsv, 8, - false); + &pending_snapshot->block_rsv, + BTRFS_NEW_ROOT_ITEMS, false); if (ret) goto dec_and_free; -- 2.14.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] btrfs: reserve space for inheriting properties 2019-02-07 16:54 ` [PATCH 1/2] btrfs: reserve space for inheriting properties Josef Bacik @ 2019-02-08 6:30 ` Nikolay Borisov 2019-03-22 19:11 ` David Sterba 2019-02-13 12:37 ` Filipe Manana 1 sibling, 1 reply; 8+ messages in thread From: Nikolay Borisov @ 2019-02-08 6:30 UTC (permalink / raw) To: Josef Bacik, linux-btrfs, kernel-team On 7.02.19 г. 18:54 ч., Josef Bacik wrote: > We've been seeing errors on our build servers related to failing to > inherit inode properties. This is because we do not pre-reserve space > for them, instead trying to reserve space with NO_FLUSH at inheritance > time. NO_FLUSH can transiently fail, but we'll still complain. It's Put one or two sentences describing the implications of BTRFS_RESERVE_NO_FLUSH. I.e that it NO_FLUSH won't try to flush current metadata reservation which could result in the said transient failure. Just to give a bit more context for someone who might be reading the commit message in the future. > just an extra credit, so simply add that to the places that call > btrfs_new_inode and call it good enough. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > --- > fs/btrfs/inode.c | 78 ++++++++++++++++++++++---------------------------------- > fs/btrfs/ioctl.c | 27 ++++++++++++-------- > 2 files changed, 46 insertions(+), 59 deletions(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 6126de9b8b9c..0da4a9d6d9fe 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -59,6 +59,14 @@ struct btrfs_dio_data { > int overwrite; > }; > > +/* > + * 2 for inode item and ref > + * 2 for dir items > + * 1 for xattr if selinux is on > + * 1 for inherited properties > + */ > +#define BTRFS_NEW_INODE_ITEMS 6 Rather than having scattered defines I'd much prefer if we have an enum and over time add all distinct reservations to that enum and change btrfs_start_transaction's interface to take this enum. It will be much more descriptive than having scattered defines. > + > static const struct inode_operations btrfs_dir_inode_operations; > static const struct inode_operations btrfs_symlink_inode_operations; > static const struct inode_operations btrfs_dir_ro_inode_operations; > @@ -6479,12 +6487,7 @@ static int btrfs_mknod(struct inode *dir, struct dentry *dentry, > u64 objectid; > u64 index = 0; > > - /* > - * 2 for inode item and ref > - * 2 for dir items > - * 1 for xattr if selinux is on > - */ > - trans = btrfs_start_transaction(root, 5); > + trans = btrfs_start_transaction(root, BTRFS_NEW_INODE_ITEMS); > if (IS_ERR(trans)) > return PTR_ERR(trans); > > @@ -6543,12 +6546,7 @@ static int btrfs_create(struct inode *dir, struct dentry *dentry, > u64 objectid; > u64 index = 0; > > - /* > - * 2 for inode item and ref > - * 2 for dir items > - * 1 for xattr if selinux is on > - */ > - trans = btrfs_start_transaction(root, 5); > + trans = btrfs_start_transaction(root, BTRFS_NEW_INODE_ITEMS); > if (IS_ERR(trans)) > return PTR_ERR(trans); > > @@ -6695,12 +6693,7 @@ static int btrfs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode) > u64 objectid = 0; > u64 index = 0; > > - /* > - * 2 items for inode and ref > - * 2 items for dir items > - * 1 for xattr if selinux is on > - */ > - trans = btrfs_start_transaction(root, 5); > + trans = btrfs_start_transaction(root, BTRFS_NEW_INODE_ITEMS); > if (IS_ERR(trans)) > return PTR_ERR(trans); > > @@ -9428,14 +9421,11 @@ static int btrfs_rename_exchange(struct inode *old_dir, > down_read(&fs_info->subvol_sem); > > /* > - * We want to reserve the absolute worst case amount of items. So if > - * both inodes are subvols and we need to unlink them then that would > - * require 4 item modifications, but if they are both normal inodes it > - * would require 5 item modifications, so we'll assume their normal > - * inodes. So 5 * 2 is 10, plus 2 for the new links, so 12 total items > - * should cover the worst case number of items we'll modify. > + * The same math from btrfs_rename applies here, except we need an extra > + * 2 items for the new links. > */ > - trans = btrfs_start_transaction(root, 12); > + trans = btrfs_start_transaction(root, > + (BTRFS_NEW_INODE_ITEMS << 1) + 2); > if (IS_ERR(trans)) { > ret = PTR_ERR(trans); > goto out_notrans; > @@ -9768,19 +9758,19 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry, > if (old_ino == BTRFS_FIRST_FREE_OBJECTID) > down_read(&fs_info->subvol_sem); > /* > - * We want to reserve the absolute worst case amount of items. So if > - * both inodes are subvols and we need to unlink them then that would > - * require 4 item modifications, but if they are both normal inodes it > - * would require 5 item modifications, so we'll assume they are normal > - * inodes. So 5 * 2 is 10, plus 1 for the new link, so 11 total items > - * should cover the worst case number of items we'll modify. > - * If our rename has the whiteout flag, we need more 5 units for the > - * new inode (1 inode item, 1 inode ref, 2 dir items and 1 xattr item > - * when selinux is enabled). > + * We want to reserve the absolute worst case amount of items. Subvol > + * inodes don't have an inode item to worry about and don't have a > + * selinux attr, so we use the BTRFS_NEW_INODE_ITEMS counter for how > + * much it costs per inode to modify. Worse case we'll have to mess > + * with 2 inodes, so 2 x BTRFS_NEW_INODE_ITEMS, and then we need an > + * extra reservation for the new link. > + * > + * If our rename has the whiteout flag we need a full new inode which > + * means another set of BTRFS_NEW_INODE_ITEMS. > */ > - trans_num_items = 11; > + trans_num_items = (BTRFS_NEW_INODE_ITEMS << 1) + 1; > if (flags & RENAME_WHITEOUT) > - trans_num_items += 5; > + trans_num_items += BTRFS_NEW_INODE_ITEMS; > trans = btrfs_start_transaction(root, trans_num_items); > if (IS_ERR(trans)) { > ret = PTR_ERR(trans); > @@ -10149,14 +10139,8 @@ static int btrfs_symlink(struct inode *dir, struct dentry *dentry, > if (name_len > BTRFS_MAX_INLINE_DATA_SIZE(fs_info)) > return -ENAMETOOLONG; > > - /* > - * 2 items for inode item and ref > - * 2 items for dir items > - * 1 item for updating parent inode item > - * 1 item for the inline extent item > - * 1 item for xattr if selinux is on > - */ > - trans = btrfs_start_transaction(root, 7); > + /* 1 item for the inline extent item */ > + trans = btrfs_start_transaction(root, BTRFS_NEW_INODE_ITEMS + 1); > if (IS_ERR(trans)) > return PTR_ERR(trans); > > @@ -10427,10 +10411,8 @@ static int btrfs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode) > u64 index; > int ret = 0; > > - /* > - * 5 units required for adding orphan entry > - */ > - trans = btrfs_start_transaction(root, 5); > + /* 1 unit required for adding orphan entry */ > + trans = btrfs_start_transaction(root, BTRFS_NEW_INODE_ITEMS + 1); > if (IS_ERR(trans)) > return PTR_ERR(trans); > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index f38a659c918c..21f8ab2d8570 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -83,6 +83,17 @@ struct btrfs_ioctl_send_args_32 { > struct btrfs_ioctl_send_args_32) > #endif > > +/* > + * 1 - parent dir inode > + * 2 - dir entries > + * 1 - root item > + * 2 - root ref/backref > + * 1 - root of snapshot > + * 1 - UUID item > + * 1 - properties > + */ > +#define BTRFS_NEW_ROOT_ITEMS 9 Even if you choose to have defines currently, I insist on them being grouped in one place e.g. ctree.h > + > static int btrfs_clone(struct inode *src, struct inode *inode, > u64 off, u64 olen, u64 olen_aligned, u64 destoff, > int no_time_update); > @@ -596,7 +607,8 @@ static noinline int create_subvol(struct inode *dir, > * The same as the snapshot creation, please see the comment > * of create_snapshot(). > */ > - ret = btrfs_subvolume_reserve_metadata(root, &block_rsv, 8, false); > + ret = btrfs_subvolume_reserve_metadata(root, &block_rsv, > + BTRFS_NEW_ROOT_ITEMS, false); > if (ret) > goto fail_free; > > @@ -804,17 +816,10 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir, > > btrfs_init_block_rsv(&pending_snapshot->block_rsv, > BTRFS_BLOCK_RSV_TEMP); > - /* > - * 1 - parent dir inode > - * 2 - dir entries > - * 1 - root item > - * 2 - root ref/backref > - * 1 - root of snapshot > - * 1 - UUID item > - */ > + > ret = btrfs_subvolume_reserve_metadata(BTRFS_I(dir)->root, > - &pending_snapshot->block_rsv, 8, > - false); > + &pending_snapshot->block_rsv, > + BTRFS_NEW_ROOT_ITEMS, false); > if (ret) > goto dec_and_free; > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] btrfs: reserve space for inheriting properties 2019-02-08 6:30 ` Nikolay Borisov @ 2019-03-22 19:11 ` David Sterba 0 siblings, 0 replies; 8+ messages in thread From: David Sterba @ 2019-03-22 19:11 UTC (permalink / raw) To: Nikolay Borisov; +Cc: Josef Bacik, linux-btrfs, kernel-team On Fri, Feb 08, 2019 at 08:30:08AM +0200, Nikolay Borisov wrote: > On 7.02.19 г. 18:54 ч., Josef Bacik wrote: > > We've been seeing errors on our build servers related to failing to > > inherit inode properties. This is because we do not pre-reserve space > > for them, instead trying to reserve space with NO_FLUSH at inheritance > > time. NO_FLUSH can transiently fail, but we'll still complain. It's > > Put one or two sentences describing the implications of > BTRFS_RESERVE_NO_FLUSH. I.e that it NO_FLUSH won't try to flush current > metadata reservation which could result in the said transient failure. > Just to give a bit more context for someone who might be reading the > commit message in the future. > > > just an extra credit, so simply add that to the places that call There's not term 'credit' used in btrfs, though it has probably the same meanting as the journal credits. We should use a consistent terminology so it's transaction item. I think I'v seen 'credit' used in the other patch in a comment too. > > btrfs_new_inode and call it good enough. > > > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > --- > > fs/btrfs/inode.c | 78 ++++++++++++++++++++++---------------------------------- > > fs/btrfs/ioctl.c | 27 ++++++++++++-------- > > 2 files changed, 46 insertions(+), 59 deletions(-) > > > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > > index 6126de9b8b9c..0da4a9d6d9fe 100644 > > --- a/fs/btrfs/inode.c > > +++ b/fs/btrfs/inode.c > > @@ -59,6 +59,14 @@ struct btrfs_dio_data { > > int overwrite; > > }; > > > > +/* > > + * 2 for inode item and ref > > + * 2 for dir items > > + * 1 for xattr if selinux is on > > + * 1 for inherited properties > > + */ > > +#define BTRFS_NEW_INODE_ITEMS 6 > > Rather than having scattered defines I'd much prefer if we have an enum > and over time add all distinct reservations to that enum and change > btrfs_start_transaction's interface to take this enum. It will be much > more descriptive than having scattered defines. Some of the values are calculated based on features that can be used at that point, so I'm not sure enum would be the best thing, eg. when all variants need to be defined. Picking a short name will become hard. OTOH something like items = NEW_INODE_ITEMS if (rename_whiteout) items += RENAME_WHITEOUT_ITEMS ... looks better to me. > > + > > static const struct inode_operations btrfs_dir_inode_operations; > > static const struct inode_operations btrfs_symlink_inode_operations; > > static const struct inode_operations btrfs_dir_ro_inode_operations; > > @@ -6479,12 +6487,7 @@ static int btrfs_mknod(struct inode *dir, struct dentry *dentry, > > u64 objectid; > > u64 index = 0; > > > > - /* > > - * 2 for inode item and ref > > - * 2 for dir items > > - * 1 for xattr if selinux is on > > - */ > > - trans = btrfs_start_transaction(root, 5); > > + trans = btrfs_start_transaction(root, BTRFS_NEW_INODE_ITEMS); > > if (IS_ERR(trans)) > > return PTR_ERR(trans); > > > > @@ -6543,12 +6546,7 @@ static int btrfs_create(struct inode *dir, struct dentry *dentry, > > u64 objectid; > > u64 index = 0; > > > > - /* > > - * 2 for inode item and ref > > - * 2 for dir items > > - * 1 for xattr if selinux is on > > - */ > > - trans = btrfs_start_transaction(root, 5); > > + trans = btrfs_start_transaction(root, BTRFS_NEW_INODE_ITEMS); > > if (IS_ERR(trans)) > > return PTR_ERR(trans); > > > > @@ -6695,12 +6693,7 @@ static int btrfs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode) > > u64 objectid = 0; > > u64 index = 0; > > > > - /* > > - * 2 items for inode and ref > > - * 2 items for dir items > > - * 1 for xattr if selinux is on > > - */ > > - trans = btrfs_start_transaction(root, 5); > > + trans = btrfs_start_transaction(root, BTRFS_NEW_INODE_ITEMS); > > if (IS_ERR(trans)) > > return PTR_ERR(trans); > > > > @@ -9428,14 +9421,11 @@ static int btrfs_rename_exchange(struct inode *old_dir, > > down_read(&fs_info->subvol_sem); > > > > /* > > - * We want to reserve the absolute worst case amount of items. So if > > - * both inodes are subvols and we need to unlink them then that would > > - * require 4 item modifications, but if they are both normal inodes it > > - * would require 5 item modifications, so we'll assume their normal > > - * inodes. So 5 * 2 is 10, plus 2 for the new links, so 12 total items > > - * should cover the worst case number of items we'll modify. > > + * The same math from btrfs_rename applies here, except we need an extra > > + * 2 items for the new links. > > */ > > - trans = btrfs_start_transaction(root, 12); > > + trans = btrfs_start_transaction(root, > > + (BTRFS_NEW_INODE_ITEMS << 1) + 2); > > if (IS_ERR(trans)) { > > ret = PTR_ERR(trans); > > goto out_notrans; > > @@ -9768,19 +9758,19 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry, > > if (old_ino == BTRFS_FIRST_FREE_OBJECTID) > > down_read(&fs_info->subvol_sem); > > /* > > - * We want to reserve the absolute worst case amount of items. So if > > - * both inodes are subvols and we need to unlink them then that would > > - * require 4 item modifications, but if they are both normal inodes it > > - * would require 5 item modifications, so we'll assume they are normal > > - * inodes. So 5 * 2 is 10, plus 1 for the new link, so 11 total items > > - * should cover the worst case number of items we'll modify. > > - * If our rename has the whiteout flag, we need more 5 units for the > > - * new inode (1 inode item, 1 inode ref, 2 dir items and 1 xattr item > > - * when selinux is enabled). > > + * We want to reserve the absolute worst case amount of items. Subvol > > + * inodes don't have an inode item to worry about and don't have a > > + * selinux attr, so we use the BTRFS_NEW_INODE_ITEMS counter for how > > + * much it costs per inode to modify. Worse case we'll have to mess > > + * with 2 inodes, so 2 x BTRFS_NEW_INODE_ITEMS, and then we need an > > + * extra reservation for the new link. > > + * > > + * If our rename has the whiteout flag we need a full new inode which > > + * means another set of BTRFS_NEW_INODE_ITEMS. > > */ > > - trans_num_items = 11; > > + trans_num_items = (BTRFS_NEW_INODE_ITEMS << 1) + 1; > > if (flags & RENAME_WHITEOUT) > > - trans_num_items += 5; > > + trans_num_items += BTRFS_NEW_INODE_ITEMS; > > trans = btrfs_start_transaction(root, trans_num_items); > > if (IS_ERR(trans)) { > > ret = PTR_ERR(trans); > > @@ -10149,14 +10139,8 @@ static int btrfs_symlink(struct inode *dir, struct dentry *dentry, > > if (name_len > BTRFS_MAX_INLINE_DATA_SIZE(fs_info)) > > return -ENAMETOOLONG; > > > > - /* > > - * 2 items for inode item and ref > > - * 2 items for dir items > > - * 1 item for updating parent inode item > > - * 1 item for the inline extent item > > - * 1 item for xattr if selinux is on > > - */ > > - trans = btrfs_start_transaction(root, 7); > > + /* 1 item for the inline extent item */ > > + trans = btrfs_start_transaction(root, BTRFS_NEW_INODE_ITEMS + 1); > > if (IS_ERR(trans)) > > return PTR_ERR(trans); > > > > @@ -10427,10 +10411,8 @@ static int btrfs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode) > > u64 index; > > int ret = 0; > > > > - /* > > - * 5 units required for adding orphan entry > > - */ > > - trans = btrfs_start_transaction(root, 5); > > + /* 1 unit required for adding orphan entry */ > > + trans = btrfs_start_transaction(root, BTRFS_NEW_INODE_ITEMS + 1); > > if (IS_ERR(trans)) > > return PTR_ERR(trans); > > > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > > index f38a659c918c..21f8ab2d8570 100644 > > --- a/fs/btrfs/ioctl.c > > +++ b/fs/btrfs/ioctl.c > > @@ -83,6 +83,17 @@ struct btrfs_ioctl_send_args_32 { > > struct btrfs_ioctl_send_args_32) > > #endif > > > > +/* > > + * 1 - parent dir inode > > + * 2 - dir entries > > + * 1 - root item > > + * 2 - root ref/backref > > + * 1 - root of snapshot > > + * 1 - UUID item > > + * 1 - properties > > + */ > > +#define BTRFS_NEW_ROOT_ITEMS 9 > > Even if you choose to have defines currently, I insist on them being > grouped in one place e.g. ctree.h Absolutelly, and don't overload ctree.h anymore, trasaction.h seems to be the right place. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] btrfs: reserve space for inheriting properties 2019-02-07 16:54 ` [PATCH 1/2] btrfs: reserve space for inheriting properties Josef Bacik 2019-02-08 6:30 ` Nikolay Borisov @ 2019-02-13 12:37 ` Filipe Manana 1 sibling, 0 replies; 8+ messages in thread From: Filipe Manana @ 2019-02-13 12:37 UTC (permalink / raw) To: Josef Bacik; +Cc: linux-btrfs, kernel-team On Thu, Feb 7, 2019 at 4:57 PM Josef Bacik <josef@toxicpanda.com> wrote: > > We've been seeing errors on our build servers related to failing to > inherit inode properties. This is because we do not pre-reserve space > for them, instead trying to reserve space with NO_FLUSH at inheritance > time. NO_FLUSH can transiently fail, but we'll still complain. It's > just an extra credit, so simply add that to the places that call > btrfs_new_inode and call it good enough. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> Reviewed-by: Filipe Manana <fdmanana@suse.com> Loogs good, thanks. Just one minor comment about using bitwise left shift instead of direct multiplication by 2. > --- > fs/btrfs/inode.c | 78 ++++++++++++++++++++++---------------------------------- > fs/btrfs/ioctl.c | 27 ++++++++++++-------- > 2 files changed, 46 insertions(+), 59 deletions(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 6126de9b8b9c..0da4a9d6d9fe 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -59,6 +59,14 @@ struct btrfs_dio_data { > int overwrite; > }; > > +/* > + * 2 for inode item and ref > + * 2 for dir items > + * 1 for xattr if selinux is on > + * 1 for inherited properties > + */ > +#define BTRFS_NEW_INODE_ITEMS 6 > + > static const struct inode_operations btrfs_dir_inode_operations; > static const struct inode_operations btrfs_symlink_inode_operations; > static const struct inode_operations btrfs_dir_ro_inode_operations; > @@ -6479,12 +6487,7 @@ static int btrfs_mknod(struct inode *dir, struct dentry *dentry, > u64 objectid; > u64 index = 0; > > - /* > - * 2 for inode item and ref > - * 2 for dir items > - * 1 for xattr if selinux is on > - */ > - trans = btrfs_start_transaction(root, 5); > + trans = btrfs_start_transaction(root, BTRFS_NEW_INODE_ITEMS); > if (IS_ERR(trans)) > return PTR_ERR(trans); > > @@ -6543,12 +6546,7 @@ static int btrfs_create(struct inode *dir, struct dentry *dentry, > u64 objectid; > u64 index = 0; > > - /* > - * 2 for inode item and ref > - * 2 for dir items > - * 1 for xattr if selinux is on > - */ > - trans = btrfs_start_transaction(root, 5); > + trans = btrfs_start_transaction(root, BTRFS_NEW_INODE_ITEMS); > if (IS_ERR(trans)) > return PTR_ERR(trans); > > @@ -6695,12 +6693,7 @@ static int btrfs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode) > u64 objectid = 0; > u64 index = 0; > > - /* > - * 2 items for inode and ref > - * 2 items for dir items > - * 1 for xattr if selinux is on > - */ > - trans = btrfs_start_transaction(root, 5); > + trans = btrfs_start_transaction(root, BTRFS_NEW_INODE_ITEMS); > if (IS_ERR(trans)) > return PTR_ERR(trans); > > @@ -9428,14 +9421,11 @@ static int btrfs_rename_exchange(struct inode *old_dir, > down_read(&fs_info->subvol_sem); > > /* > - * We want to reserve the absolute worst case amount of items. So if > - * both inodes are subvols and we need to unlink them then that would > - * require 4 item modifications, but if they are both normal inodes it > - * would require 5 item modifications, so we'll assume their normal > - * inodes. So 5 * 2 is 10, plus 2 for the new links, so 12 total items > - * should cover the worst case number of items we'll modify. > + * The same math from btrfs_rename applies here, except we need an extra > + * 2 items for the new links. > */ > - trans = btrfs_start_transaction(root, 12); > + trans = btrfs_start_transaction(root, > + (BTRFS_NEW_INODE_ITEMS << 1) + 2); > if (IS_ERR(trans)) { > ret = PTR_ERR(trans); > goto out_notrans; > @@ -9768,19 +9758,19 @@ static int btrfs_rename(struct inode *old_dir, struct dentry *old_dentry, > if (old_ino == BTRFS_FIRST_FREE_OBJECTID) > down_read(&fs_info->subvol_sem); > /* > - * We want to reserve the absolute worst case amount of items. So if > - * both inodes are subvols and we need to unlink them then that would > - * require 4 item modifications, but if they are both normal inodes it > - * would require 5 item modifications, so we'll assume they are normal > - * inodes. So 5 * 2 is 10, plus 1 for the new link, so 11 total items > - * should cover the worst case number of items we'll modify. > - * If our rename has the whiteout flag, we need more 5 units for the > - * new inode (1 inode item, 1 inode ref, 2 dir items and 1 xattr item > - * when selinux is enabled). > + * We want to reserve the absolute worst case amount of items. Subvol > + * inodes don't have an inode item to worry about and don't have a > + * selinux attr, so we use the BTRFS_NEW_INODE_ITEMS counter for how > + * much it costs per inode to modify. Worse case we'll have to mess > + * with 2 inodes, so 2 x BTRFS_NEW_INODE_ITEMS, and then we need an > + * extra reservation for the new link. > + * > + * If our rename has the whiteout flag we need a full new inode which > + * means another set of BTRFS_NEW_INODE_ITEMS. > */ > - trans_num_items = 11; > + trans_num_items = (BTRFS_NEW_INODE_ITEMS << 1) + 1; Could be made more obvious by multiplying by 2 instead of bitwise left shift. > if (flags & RENAME_WHITEOUT) > - trans_num_items += 5; > + trans_num_items += BTRFS_NEW_INODE_ITEMS; > trans = btrfs_start_transaction(root, trans_num_items); > if (IS_ERR(trans)) { > ret = PTR_ERR(trans); > @@ -10149,14 +10139,8 @@ static int btrfs_symlink(struct inode *dir, struct dentry *dentry, > if (name_len > BTRFS_MAX_INLINE_DATA_SIZE(fs_info)) > return -ENAMETOOLONG; > > - /* > - * 2 items for inode item and ref > - * 2 items for dir items > - * 1 item for updating parent inode item > - * 1 item for the inline extent item > - * 1 item for xattr if selinux is on > - */ > - trans = btrfs_start_transaction(root, 7); > + /* 1 item for the inline extent item */ > + trans = btrfs_start_transaction(root, BTRFS_NEW_INODE_ITEMS + 1); > if (IS_ERR(trans)) > return PTR_ERR(trans); > > @@ -10427,10 +10411,8 @@ static int btrfs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode) > u64 index; > int ret = 0; > > - /* > - * 5 units required for adding orphan entry > - */ > - trans = btrfs_start_transaction(root, 5); > + /* 1 unit required for adding orphan entry */ > + trans = btrfs_start_transaction(root, BTRFS_NEW_INODE_ITEMS + 1); > if (IS_ERR(trans)) > return PTR_ERR(trans); > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index f38a659c918c..21f8ab2d8570 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -83,6 +83,17 @@ struct btrfs_ioctl_send_args_32 { > struct btrfs_ioctl_send_args_32) > #endif > > +/* > + * 1 - parent dir inode > + * 2 - dir entries > + * 1 - root item > + * 2 - root ref/backref > + * 1 - root of snapshot > + * 1 - UUID item > + * 1 - properties > + */ > +#define BTRFS_NEW_ROOT_ITEMS 9 > + > static int btrfs_clone(struct inode *src, struct inode *inode, > u64 off, u64 olen, u64 olen_aligned, u64 destoff, > int no_time_update); > @@ -596,7 +607,8 @@ static noinline int create_subvol(struct inode *dir, > * The same as the snapshot creation, please see the comment > * of create_snapshot(). > */ > - ret = btrfs_subvolume_reserve_metadata(root, &block_rsv, 8, false); > + ret = btrfs_subvolume_reserve_metadata(root, &block_rsv, > + BTRFS_NEW_ROOT_ITEMS, false); > if (ret) > goto fail_free; > > @@ -804,17 +816,10 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir, > > btrfs_init_block_rsv(&pending_snapshot->block_rsv, > BTRFS_BLOCK_RSV_TEMP); > - /* > - * 1 - parent dir inode > - * 2 - dir entries > - * 1 - root item > - * 2 - root ref/backref > - * 1 - root of snapshot > - * 1 - UUID item > - */ > + > ret = btrfs_subvolume_reserve_metadata(BTRFS_I(dir)->root, > - &pending_snapshot->block_rsv, 8, > - false); > + &pending_snapshot->block_rsv, > + BTRFS_NEW_ROOT_ITEMS, false); > if (ret) > goto dec_and_free; > > -- > 2.14.3 > -- Filipe David Manana, “Whether you think you can, or you think you can't — you're right.” ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] btrfs: use the existing credit for our first prop 2019-02-07 16:54 [PATCH 0/2] Reserve more space for property inheritance Josef Bacik 2019-02-07 16:54 ` [PATCH 1/2] btrfs: reserve space for inheriting properties Josef Bacik @ 2019-02-07 16:54 ` Josef Bacik 2019-02-13 12:38 ` Filipe Manana 2019-04-26 14:29 ` David Sterba 1 sibling, 2 replies; 8+ messages in thread From: Josef Bacik @ 2019-02-07 16:54 UTC (permalink / raw) To: linux-btrfs, kernel-team We're now reserving an extra items worth of space for property inheritance. We only have one property at the moment so this covers us, but if we add more in the future this will allow us to not get bitten by the extra space reservation. If we do add more properties in the future we should re-visit how we calculate the space reservation needs by the callers. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/props.c | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c index dc6140013ae8..b3d22fef8c92 100644 --- a/fs/btrfs/props.c +++ b/fs/btrfs/props.c @@ -291,6 +291,7 @@ static int inherit_props(struct btrfs_trans_handle *trans, struct btrfs_fs_info *fs_info = root->fs_info; int ret; int i; + bool need_reserve = false; if (!test_bit(BTRFS_INODE_HAS_PROPS, &BTRFS_I(parent)->runtime_flags)) @@ -308,16 +309,31 @@ static int inherit_props(struct btrfs_trans_handle *trans, if (!value) continue; - num_bytes = btrfs_calc_trans_metadata_size(fs_info, 1); - ret = btrfs_block_rsv_add(root, trans->block_rsv, - num_bytes, BTRFS_RESERVE_NO_FLUSH); - if (ret) - goto out; + /* + * Currently callers should be reserving 1 credit for + * properties, since we only have 1 property that we currently + * support. If we add more in the future we need to try and + * reserve more space for them. But we should also revisit how + * we do space reservations if we do add more properties in the + * future. + */ + if (need_reserve) { + num_bytes = btrfs_calc_trans_metadata_size(fs_info, 1); + ret = btrfs_block_rsv_add(root, trans->block_rsv, + num_bytes, + BTRFS_RESERVE_NO_FLUSH); + if (ret) + goto out; + } ret = __btrfs_set_prop(trans, inode, h->xattr_name, value, strlen(value), 0); - btrfs_block_rsv_release(fs_info, trans->block_rsv, num_bytes); - if (ret) - goto out; + if (need_reserve) { + btrfs_block_rsv_release(fs_info, trans->block_rsv, + num_bytes); + if (ret) + goto out; + } + need_reserve = true; } ret = 0; out: -- 2.14.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] btrfs: use the existing credit for our first prop 2019-02-07 16:54 ` [PATCH 2/2] btrfs: use the existing credit for our first prop Josef Bacik @ 2019-02-13 12:38 ` Filipe Manana 2019-04-26 14:29 ` David Sterba 1 sibling, 0 replies; 8+ messages in thread From: Filipe Manana @ 2019-02-13 12:38 UTC (permalink / raw) To: Josef Bacik; +Cc: linux-btrfs, kernel-team On Thu, Feb 7, 2019 at 4:57 PM Josef Bacik <josef@toxicpanda.com> wrote: > > We're now reserving an extra items worth of space for property > inheritance. We only have one property at the moment so this covers us, > but if we add more in the future this will allow us to not get bitten by > the extra space reservation. If we do add more properties in the future > we should re-visit how we calculate the space reservation needs by the > callers. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> Reviewed-by: Filipe Manana <fdmanana@suse.com> Looks good, thanks. > --- > fs/btrfs/props.c | 32 ++++++++++++++++++++++++-------- > 1 file changed, 24 insertions(+), 8 deletions(-) > > diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c > index dc6140013ae8..b3d22fef8c92 100644 > --- a/fs/btrfs/props.c > +++ b/fs/btrfs/props.c > @@ -291,6 +291,7 @@ static int inherit_props(struct btrfs_trans_handle *trans, > struct btrfs_fs_info *fs_info = root->fs_info; > int ret; > int i; > + bool need_reserve = false; > > if (!test_bit(BTRFS_INODE_HAS_PROPS, > &BTRFS_I(parent)->runtime_flags)) > @@ -308,16 +309,31 @@ static int inherit_props(struct btrfs_trans_handle *trans, > if (!value) > continue; > > - num_bytes = btrfs_calc_trans_metadata_size(fs_info, 1); > - ret = btrfs_block_rsv_add(root, trans->block_rsv, > - num_bytes, BTRFS_RESERVE_NO_FLUSH); > - if (ret) > - goto out; > + /* > + * Currently callers should be reserving 1 credit for > + * properties, since we only have 1 property that we currently > + * support. If we add more in the future we need to try and > + * reserve more space for them. But we should also revisit how > + * we do space reservations if we do add more properties in the > + * future. > + */ > + if (need_reserve) { > + num_bytes = btrfs_calc_trans_metadata_size(fs_info, 1); > + ret = btrfs_block_rsv_add(root, trans->block_rsv, > + num_bytes, > + BTRFS_RESERVE_NO_FLUSH); > + if (ret) > + goto out; > + } > ret = __btrfs_set_prop(trans, inode, h->xattr_name, > value, strlen(value), 0); > - btrfs_block_rsv_release(fs_info, trans->block_rsv, num_bytes); > - if (ret) > - goto out; > + if (need_reserve) { > + btrfs_block_rsv_release(fs_info, trans->block_rsv, > + num_bytes); > + if (ret) > + goto out; > + } > + need_reserve = true; > } > ret = 0; > out: > -- > 2.14.3 > -- Filipe David Manana, “Whether you think you can, or you think you can't — you're right.” ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] btrfs: use the existing credit for our first prop 2019-02-07 16:54 ` [PATCH 2/2] btrfs: use the existing credit for our first prop Josef Bacik 2019-02-13 12:38 ` Filipe Manana @ 2019-04-26 14:29 ` David Sterba 1 sibling, 0 replies; 8+ messages in thread From: David Sterba @ 2019-04-26 14:29 UTC (permalink / raw) To: Josef Bacik; +Cc: linux-btrfs, kernel-team On Thu, Feb 07, 2019 at 11:54:26AM -0500, Josef Bacik wrote: > We're now reserving an extra items worth of space for property > inheritance. We only have one property at the moment so this covers us, > but if we add more in the future this will allow us to not get bitten by > the extra space reservation. If we do add more properties in the future > we should re-visit how we calculate the space reservation needs by the > callers. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> There were xattr and property code cleanups, I've applied this patch on top of that as the logic hasn't changed, only the context. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-04-26 14:28 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-07 16:54 [PATCH 0/2] Reserve more space for property inheritance Josef Bacik 2019-02-07 16:54 ` [PATCH 1/2] btrfs: reserve space for inheriting properties Josef Bacik 2019-02-08 6:30 ` Nikolay Borisov 2019-03-22 19:11 ` David Sterba 2019-02-13 12:37 ` Filipe Manana 2019-02-07 16:54 ` [PATCH 2/2] btrfs: use the existing credit for our first prop Josef Bacik 2019-02-13 12:38 ` Filipe Manana 2019-04-26 14:29 ` David Sterba
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).