| Message ID | 20210222164047.978768-7-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Qgroup/delayed node related fixes | expand |
On Mon, Feb 22, 2021 at 06:40:47PM +0200, Nikolay Borisov wrote: > btrfs_block_rsv_add can return only ENOSPC since it's called with > NO_FLUSH modifier. This so simplify the logic in > btrfs_delayed_inode_reserve_metadata to exploit this invariant. This seems quite fragile, it's not straightforward to see from the context that the NO_FLUSH code will always return ENOSPC. I followed a few calls down from btrfs_block_rsv_add and it's well hidden inside __reserve_bytes. So in case it's an invariant I'd rather add an assertion, ie. ASSERT(ret == 0 || ret == -ENOSPC) so at least we know when this gets broken. Otherwise looks ok.
On 1.03.21 г. 18:15 ч., David Sterba wrote: > On Mon, Feb 22, 2021 at 06:40:47PM +0200, Nikolay Borisov wrote: >> btrfs_block_rsv_add can return only ENOSPC since it's called with >> NO_FLUSH modifier. This so simplify the logic in >> btrfs_delayed_inode_reserve_metadata to exploit this invariant. > > This seems quite fragile, it's not straightforward to see from the > context that the NO_FLUSH code will always return ENOSPC. I followed a > few calls down from btrfs_block_rsv_add and it's well hidden inside > __reserve_bytes. So in case it's an invariant I'd rather add an > assertion, ie. ASSERT(ret == 0 || ret == -ENOSPC) so at least we know > when this gets broken. Otherwise looks ok. > Fair enough, I'm fine with it. In any case we no longer return eagain when reserving. either we succeed or we return ENOSPC - either because we don't have space and we can't flush or because even after the flushing machinery did its work a ticket still couldn't be satisfied, in which case we failed it hence ENOSPC got returned.
On Mon, Mar 01, 2021 at 06:20:29PM +0200, Nikolay Borisov wrote: > > > On 1.03.21 г. 18:15 ч., David Sterba wrote: > > On Mon, Feb 22, 2021 at 06:40:47PM +0200, Nikolay Borisov wrote: > >> btrfs_block_rsv_add can return only ENOSPC since it's called with > >> NO_FLUSH modifier. This so simplify the logic in > >> btrfs_delayed_inode_reserve_metadata to exploit this invariant. > > > > This seems quite fragile, it's not straightforward to see from the > > context that the NO_FLUSH code will always return ENOSPC. I followed a > > few calls down from btrfs_block_rsv_add and it's well hidden inside > > __reserve_bytes. So in case it's an invariant I'd rather add an > > assertion, ie. ASSERT(ret == 0 || ret == -ENOSPC) so at least we know > > when this gets broken. Otherwise looks ok. > > Fair enough, I'm fine with it. In any case we no longer return eagain > when reserving. either we succeed or we return ENOSPC - either because > we don't have space and we can't flush or because even after the > flushing machinery did its work a ticket still couldn't be satisfied, in > which case we failed it hence ENOSPC got returned. Yeah but this is a precaution when somebody reworks the flushing logic yet another time and suddenly EAGAIN or whatever else can become the return code again. Hunting such bugs can be quite difficult as it depends on the runtime state and the space stat on disk etc.
diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c index 875daca63d5d..92843105ebd8 100644 --- a/fs/btrfs/delayed-inode.c +++ b/fs/btrfs/delayed-inode.c @@ -632,29 +632,12 @@ static int btrfs_delayed_inode_reserve_metadata( return ret; ret = btrfs_block_rsv_add(root, dst_rsv, num_bytes, BTRFS_RESERVE_NO_FLUSH); - /* - * Since we're under a transaction reserve_metadata_bytes could - * try to commit the transaction which will make it return - * EAGAIN to make us stop the transaction we have, so return - * ENOSPC instead so that btrfs_dirty_inode knows what to do. - */ - if (ret == -EAGAIN) { - ret = -ENOSPC; - btrfs_qgroup_free_meta_prealloc(root, num_bytes); - } - if (!ret) { - node->bytes_reserved = num_bytes; - trace_btrfs_space_reservation(fs_info, - "delayed_inode", - node->inode_id, - num_bytes, 1); - } else { + if (ret) btrfs_qgroup_free_meta_prealloc(root, num_bytes); - } - return ret; + } else { + ret = btrfs_block_rsv_migrate(src_rsv, dst_rsv, num_bytes, true); } - ret = btrfs_block_rsv_migrate(src_rsv, dst_rsv, num_bytes, true); if (!ret) { trace_btrfs_space_reservation(fs_info, "delayed_inode", node->inode_id, num_bytes, 1);
btrfs_block_rsv_add can return only ENOSPC since it's called with NO_FLUSH modifier. This so simplify the logic in btrfs_delayed_inode_reserve_metadata to exploit this invariant. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/btrfs/delayed-inode.c | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-)