| Message ID | 18b2ae0948a035aa809ba38641439e2d4167ca29.1638477127.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Free space tree space reservation fixes | expand |
On Thu, Dec 02, 2021 at 03:34:32PM -0500, Josef Bacik wrote: > Filipe reported a problem where sometimes he'd get an ENOSPC abort when > running delayed refs with generic/619 and the free space tree enabled. > This is partly because we do not reserve space for modifying the free > space tree, nor do we have a block rsv associated with that tree. > > The delayed_refs_rsv tracks the amount of space required to run delayed > refs. This means 1 modification means 1 change to the extent root. > With the free space tree this turns into 2 changes, because modifying 1 > extent means updating the extent tree and potentially updating the free > space tree to either remove that entry or add the free space. Thus if > we have the FST enabled, simply double the reservation size for our > modification. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > --- > fs/btrfs/block-rsv.c | 1 + > fs/btrfs/delayed-ref.c | 22 ++++++++++++++++++++++ > 2 files changed, 23 insertions(+) > > diff --git a/fs/btrfs/block-rsv.c b/fs/btrfs/block-rsv.c > index b3086f252ad0..b3ee49b0b1e8 100644 > --- a/fs/btrfs/block-rsv.c > +++ b/fs/btrfs/block-rsv.c > @@ -426,6 +426,7 @@ void btrfs_init_root_block_rsv(struct btrfs_root *root) > switch (root->root_key.objectid) { > case BTRFS_CSUM_TREE_OBJECTID: > case BTRFS_EXTENT_TREE_OBJECTID: > + case BTRFS_FREE_SPACE_TREE_OBJECTID: > root->block_rsv = &fs_info->delayed_refs_rsv; > break; > case BTRFS_ROOT_TREE_OBJECTID: > diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c > index da9d20813147..533521be8fdf 100644 > --- a/fs/btrfs/delayed-ref.c > +++ b/fs/btrfs/delayed-ref.c > @@ -84,6 +84,17 @@ void btrfs_delayed_refs_rsv_release(struct btrfs_fs_info *fs_info, int nr) > u64 num_bytes = btrfs_calc_insert_metadata_size(fs_info, nr); > u64 released = 0; > > + /* > + * We have to check the mount option here because we could be enabling > + * the free space tree for the first time and don't have the compat_ro > + * option set yet. > + * > + * We need extra reservations if we have the free space tree because > + * we'll have to modify that tree as well. > + */ > + if (btrfs_test_opt(fs_info, FREE_SPACE_TREE)) > + num_bytes <<= 1; > + > released = btrfs_block_rsv_release(fs_info, block_rsv, num_bytes, NULL); > if (released) > trace_btrfs_space_reservation(fs_info, "delayed_refs_rsv", > @@ -108,6 +119,17 @@ void btrfs_update_delayed_refs_rsv(struct btrfs_trans_handle *trans) > > num_bytes = btrfs_calc_insert_metadata_size(fs_info, > trans->delayed_ref_updates); > + /* > + * We have to check the mount option here because we could be enabling > + * the free space tree for the first time and don't have the compat_ro > + * option set yet. > + * > + * We need extra reservations if we have the free space tree because > + * we'll have to modify that tree as well. > + */ > + if (btrfs_test_opt(fs_info, FREE_SPACE_TREE)) > + num_bytes <<= 1; Don't we need to bump the minimum (limit variable) number of bytes at btrfs_delayed_refs_rsv_refill() as well? I don't see why not. Thanks. > + > spin_lock(&delayed_rsv->lock); > delayed_rsv->size += num_bytes; > delayed_rsv->full = 0; > -- > 2.26.3 >
On Mon, Dec 06, 2021 at 10:44:51AM +0000, Filipe Manana wrote: > On Thu, Dec 02, 2021 at 03:34:32PM -0500, Josef Bacik wrote: > > Filipe reported a problem where sometimes he'd get an ENOSPC abort when > > running delayed refs with generic/619 and the free space tree enabled. > > This is partly because we do not reserve space for modifying the free > > space tree, nor do we have a block rsv associated with that tree. > > > > The delayed_refs_rsv tracks the amount of space required to run delayed > > refs. This means 1 modification means 1 change to the extent root. > > With the free space tree this turns into 2 changes, because modifying 1 > > extent means updating the extent tree and potentially updating the free > > space tree to either remove that entry or add the free space. Thus if > > we have the FST enabled, simply double the reservation size for our > > modification. > > > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > --- > > fs/btrfs/block-rsv.c | 1 + > > fs/btrfs/delayed-ref.c | 22 ++++++++++++++++++++++ > > 2 files changed, 23 insertions(+) > > > > diff --git a/fs/btrfs/block-rsv.c b/fs/btrfs/block-rsv.c > > index b3086f252ad0..b3ee49b0b1e8 100644 > > --- a/fs/btrfs/block-rsv.c > > +++ b/fs/btrfs/block-rsv.c > > @@ -426,6 +426,7 @@ void btrfs_init_root_block_rsv(struct btrfs_root *root) > > switch (root->root_key.objectid) { > > case BTRFS_CSUM_TREE_OBJECTID: > > case BTRFS_EXTENT_TREE_OBJECTID: > > + case BTRFS_FREE_SPACE_TREE_OBJECTID: > > root->block_rsv = &fs_info->delayed_refs_rsv; > > break; > > case BTRFS_ROOT_TREE_OBJECTID: > > diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c > > index da9d20813147..533521be8fdf 100644 > > --- a/fs/btrfs/delayed-ref.c > > +++ b/fs/btrfs/delayed-ref.c > > @@ -84,6 +84,17 @@ void btrfs_delayed_refs_rsv_release(struct btrfs_fs_info *fs_info, int nr) > > u64 num_bytes = btrfs_calc_insert_metadata_size(fs_info, nr); > > u64 released = 0; > > > > + /* > > + * We have to check the mount option here because we could be enabling > > + * the free space tree for the first time and don't have the compat_ro > > + * option set yet. > > + * > > + * We need extra reservations if we have the free space tree because > > + * we'll have to modify that tree as well. > > + */ > > + if (btrfs_test_opt(fs_info, FREE_SPACE_TREE)) > > + num_bytes <<= 1; > > + > > released = btrfs_block_rsv_release(fs_info, block_rsv, num_bytes, NULL); > > if (released) > > trace_btrfs_space_reservation(fs_info, "delayed_refs_rsv", > > @@ -108,6 +119,17 @@ void btrfs_update_delayed_refs_rsv(struct btrfs_trans_handle *trans) > > > > num_bytes = btrfs_calc_insert_metadata_size(fs_info, > > trans->delayed_ref_updates); > > + /* > > + * We have to check the mount option here because we could be enabling > > + * the free space tree for the first time and don't have the compat_ro > > + * option set yet. > > + * > > + * We need extra reservations if we have the free space tree because > > + * we'll have to modify that tree as well. > > + */ > > + if (btrfs_test_opt(fs_info, FREE_SPACE_TREE)) > > + num_bytes <<= 1; > > Don't we need to bump the minimum (limit variable) number of bytes at > btrfs_delayed_refs_rsv_refill() as well? > > I don't see why not. > Because refill is about adding more space to keep up with usage. We're not adding space at that point. These things here are to make sure ->size is correct. Refill is about making sure ->reserved == ->size. In this case we're just trying to add the smallest unit possible, min(1 items worth of modificaitons, ->size - >reserved). Thanks, Josef
diff --git a/fs/btrfs/block-rsv.c b/fs/btrfs/block-rsv.c index b3086f252ad0..b3ee49b0b1e8 100644 --- a/fs/btrfs/block-rsv.c +++ b/fs/btrfs/block-rsv.c @@ -426,6 +426,7 @@ void btrfs_init_root_block_rsv(struct btrfs_root *root) switch (root->root_key.objectid) { case BTRFS_CSUM_TREE_OBJECTID: case BTRFS_EXTENT_TREE_OBJECTID: + case BTRFS_FREE_SPACE_TREE_OBJECTID: root->block_rsv = &fs_info->delayed_refs_rsv; break; case BTRFS_ROOT_TREE_OBJECTID: diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c index da9d20813147..533521be8fdf 100644 --- a/fs/btrfs/delayed-ref.c +++ b/fs/btrfs/delayed-ref.c @@ -84,6 +84,17 @@ void btrfs_delayed_refs_rsv_release(struct btrfs_fs_info *fs_info, int nr) u64 num_bytes = btrfs_calc_insert_metadata_size(fs_info, nr); u64 released = 0; + /* + * We have to check the mount option here because we could be enabling + * the free space tree for the first time and don't have the compat_ro + * option set yet. + * + * We need extra reservations if we have the free space tree because + * we'll have to modify that tree as well. + */ + if (btrfs_test_opt(fs_info, FREE_SPACE_TREE)) + num_bytes <<= 1; + released = btrfs_block_rsv_release(fs_info, block_rsv, num_bytes, NULL); if (released) trace_btrfs_space_reservation(fs_info, "delayed_refs_rsv", @@ -108,6 +119,17 @@ void btrfs_update_delayed_refs_rsv(struct btrfs_trans_handle *trans) num_bytes = btrfs_calc_insert_metadata_size(fs_info, trans->delayed_ref_updates); + /* + * We have to check the mount option here because we could be enabling + * the free space tree for the first time and don't have the compat_ro + * option set yet. + * + * We need extra reservations if we have the free space tree because + * we'll have to modify that tree as well. + */ + if (btrfs_test_opt(fs_info, FREE_SPACE_TREE)) + num_bytes <<= 1; + spin_lock(&delayed_rsv->lock); delayed_rsv->size += num_bytes; delayed_rsv->full = 0;
Filipe reported a problem where sometimes he'd get an ENOSPC abort when running delayed refs with generic/619 and the free space tree enabled. This is partly because we do not reserve space for modifying the free space tree, nor do we have a block rsv associated with that tree. The delayed_refs_rsv tracks the amount of space required to run delayed refs. This means 1 modification means 1 change to the extent root. With the free space tree this turns into 2 changes, because modifying 1 extent means updating the extent tree and potentially updating the free space tree to either remove that entry or add the free space. Thus if we have the FST enabled, simply double the reservation size for our modification. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/block-rsv.c | 1 + fs/btrfs/delayed-ref.c | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+)