[PATCH 0/2] Reserve more space for property inheritance

[PATCH 0/2] Reserve more space for property inheritance

[Josef Bacik]

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

[PATCH 1/2] btrfs: reserve space for inheriting properties

[Josef Bacik]

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

[PATCH 2/2] btrfs: use the existing credit for our first prop

[Josef Bacik]

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

Re: [PATCH 1/2] btrfs: reserve space for inheriting properties

[Nikolay Borisov]

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;
>  
> 

Re: [PATCH 1/2] btrfs: reserve space for inheriting properties

[Filipe Manana]

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.”

Re: [PATCH 2/2] btrfs: use the existing credit for our first prop

[Filipe Manana]

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.”

Re: [PATCH 1/2] btrfs: reserve space for inheriting properties

[David Sterba]

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.

Re: [PATCH 2/2] btrfs: use the existing credit for our first prop

[David Sterba]

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.