ext4: don't use quota reservation for speculative metadata blocks
RFC PATCH 1/1 ext4: don't use quota reservation for speculative metadata blocks
Because we can badly over-reserve metadata when we calculate worst-case, it complicates things for quota, since we must reserve and then claim later, retry on EDQUOT, etc. Quota is also a generally smaller pool than fs free blocks, so this over-reservation hurts more, and more often.
I'm of the opinion that it's not the worst thing to allow metadata to push a user slightly over quota, if we can simplify the code by doing so.
The patch below stops the speculative quota-charging for worst-case metadata requirements, and just charges quota when the blocks are allocated at writeout. It also is able to remove the try-again loop on EDQUOT.
In the longer run I'd like to even consider not charging speculative metadata against the superblock counters, and use a reserved space pool similar to what XFS does, but this change could maybe stand on its own, too.
---
balloc.c | 5 +++-- inode.c | 63 +++++++++++++++++++++------------------------------------------ 2 files changed, 24 insertions(+), 44 deletions(-)
fs/ext4/balloc.c +3 -2
@@ -604,13 +604,14 @@ ext4_fsblk_t ext4_new_meta_blocks(handle_t *handle, struct inode *inode, ret =ext4_mb_new_blocks (handle, &ar, errp);if (count) *count = ar.len; -/* -* Account for the allocated meta blocks +* Account for the allocated meta blocks. We will never +* fail EDQUOT for metdata, but we do account for it. */ if (!(*errp) &&EXT4_I (inode)->i_delalloc_reserved_flag) {spin_lock (&EXT4_I (inode)->i_block_reservation_lock);EXT4_I (inode)->i_allocated_meta_blocks += ar.len; +vfs_dq_alloc_block (inode, ar.len);spin_unlock (&EXT4_I (inode)->i_block_reservation_lock); }return ret;
fs/ext4/inode.c +21 -26
118 lines not on this page · raw message
Jan Kara
reviewerQuoted 5 lines
Because we can badly over-reserve metadata when we calculate worst-case, it complicates things for quota, since we must reserve and then claim later, retry on EDQUOT, etc. Quota is also a generally smaller pool than fs free blocks, so this over-reservation hurts more, and more often.
Yes, it's kind of nasty...
Quoted 3 lines
I'm of the opinion that it's not the worst thing to allow metadata to push a user slightly over quota, if we can simplify the code by doing so.
Well, the code simplification isn't be such a big deal in this particular
case at least for me - the false failures / warnings seem like a worse problem to me.
Quoted 4 lines
In the longer run I'd like to even consider not charging speculative metadata against the superblock counters, and use a reserved space pool similar to what XFS does, but this change could maybe stand on its own, too.
This is going to be tough because you just cannot afford to fail
an allocation as soon as you've accepted data to write. So you have to do some computations anyway to make sure you can always find space for metadata.
Quoted 8 lines
fs/ext4/balloc.c +0
-0@@ -604,13 +604,14 @@ ext4_fsblk_t ext4_new_meta_blocks(handle_t *handle, struct inode *inode, ret =ext4_mb_new_blocks (handle, &ar, errp);if (count) *count = ar.len;
fs/ext4/balloc.c ext4_fsblk_t ext4_new_meta_blocks(handle_t *handle, struct inode *inode, · line 614
Quoted 10 lines
fs/ext4/balloc.c +3
-2-/* -* Account for the allocated meta blocks +* Account for the allocated meta blocks. We will never +* fail EDQUOT for metdata, but we do account for it. */ if (!(*errp) &&EXT4_I (inode)->i_delalloc_reserved_flag) {spin_lock (&EXT4_I (inode)->i_block_reservation_lock);EXT4_I (inode)->i_allocated_meta_blocks += ar.len; +vfs_dq_alloc_block (inode, ar.len);
This won't work - vfs_dq_alloc_block just refuses to allocate blocks
if you'd go over limit. You probably have to introduce new flag to the function to don't do limit checks... BTW: Christoph Hellwig and Dmitry are doing some quota cleanups which change the interface so it'd be better if you based quota patches on top of my linux-fs-2.6 tree.
Also you shouldn't call vfs_dq_alloc_block inside a spinlock since it
can sleep.
Otherwise the patch looks fine, although all the counter stuff looks messy
so I'm not so sure I didn't miss some subtlety.
Honza
Signature · 2 lines
Jan Kara <[email protected]> SuSE CR Labs
Quoted 5 lines · Jan Kara
Quoted 5 lines
Because we can badly over-reserve metadata when we calculate worst-case, it complicates things for quota, since we must reserve and then claim later, retry on EDQUOT, etc. Quota is also a generally smaller pool than fs free blocks, so this over-reservation hurts more, and more often.
Quoted 8 lines
Yes, it's kind of nasty...
Quoted 3 lines
I'm of the opinion that it's not the worst thing to allow metadata to push a user slightly over quota, if we can simplify the code by doing so.
Well, the code simplification isn't be such a big deal in this particular
case at least for me - the false failures / warnings seem like a worse problem to me.
Well, good point. :)
Quoted 21 lines
Quoted 4 lines
In the longer run I'd like to even consider not charging speculative metadata against the superblock counters, and use a reserved space pool similar to what XFS does, but this change could maybe stand on its own, too.
This is going to be tough because you just cannot afford to fail
an allocation as soon as you've accepted data to write. So you have to do some computations anyway to make sure you can always find space for metadata.Quoted 12 lines
fs/ext4/balloc.c +1
-2@@ -604,13 +604,14 @@ ext4_fsblk_t ext4_new_meta_blocks(handle_t *handle, struct inode *inode, ret =ext4_mb_new_blocks (handle, &ar, errp);if (count) *count = ar.len; -/* -* Account for the allocated meta blocks +* Account for the allocated meta blocks. We will never
Quoted 8 lines
fs/ext4/balloc.c ext4_fsblk_t ext4_new_meta_blocks(handle_t *handle, struct inode *inode, · line 614
Quoted 6 lines
fs/ext4/balloc.c +2
-0+* fail EDQUOT for metdata, but we do account for it. */ if (!(*errp) &&EXT4_I (inode)->i_delalloc_reserved_flag) {spin_lock (&EXT4_I (inode)->i_block_reservation_lock);EXT4_I (inode)->i_allocated_meta_blocks += ar.len; +vfs_dq_alloc_block (inode, ar.len);This won't work -
vfs_dq_alloc_blockjust refuses to allocate blocks
if you'd go over limit. You probably have to introduce new flag to
Oh, missed that. Bummer, ok.
Quoted 3 lines
the function to don't do limit checks... BTW: Christoph Hellwig and Dmitry are doing some quota cleanups which change the interface so it'd be better if you based quota patches on top of my linux-fs-2.6 tree.
OK
Quoted 2 lines
Also you shouldn't call
vfs_dq_alloc_blockinside a spinlock since it
can sleep.
Oh, oops, I guess I knew that ...
Quoted 2 lines
Otherwise the patch looks fine, although all the counter stuff looks messy
so I'm not so sure I didn't miss some subtlety.
The counter stuff is still messy because the superblock needs it - if that's the messiness you refer to ...
Thanks,
-Eric
Quoted 1 line
Honza