Hi all:
I'm looking in ext4_mb_use_preallocated() and am seeing something odd.
First we look through the inode prealloc list, and see if we have a
preallocation that satisfies the allocation context:
/* all fields in this condition don't change,
* so we can skip locking for them */
if (ac->ac_o_ex.fe_logical < pa->pa_lstart ||
ac->ac_o_ex.fe_logical >= pa->pa_lstart + pa->pa_len)
continue;
/* non-extent files can't have physical blocks past 2^32 */
if (!(EXT4_I(ac->ac_inode)->i_flags & EXT4_EXTENTS_FL) &&
pa->pa_pstart + pa->pa_len > EXT4_MAX_BLOCK_FILE_PHYS)
continue;
/* found preallocated blocks, use them */
spin_lock(&pa->pa_lock);
if (pa->pa_deleted == 0 && pa->pa_free) {
=> Now we're good, and have an AC that satisfies us.
=> We call ext4_mb_use_inode_pa(ac, pa);
But ext4_mb_use_inode_pa() has this:
BUG_ON(pa->pa_free < len);
Nowhere do we check the 'pa_free' value to decide if this preallocation is
okay to use.
Further down in ext4_mb_use_preallocated() we check the locality group
prealloc list; for this, we DO check pa_free:
spin_lock(&pa->pa_lock);
if (pa->pa_deleted == 0 &&
pa->pa_free >= ac->ac_o_ex.fe_len) {
cpa = ext4_mb_check_group_pa(goal_block,
pa, cpa);
So my question is: Is it a bug that we don't check that an inode
preallocation has enough free blocks for the AC before we try to use it? I
have hit the BUG_ON above at least once in my testing, but I can't
characterize what the workload was at the time (nor can I reproduce it...).
Thanks,
Curt
On Tue, Oct 13, 2009 at 11:06:35AM -0700, Curt Wohlgemuth wrote:
> Hi all:
>
> I'm looking in ext4_mb_use_preallocated() and am seeing something odd.
>
> First we look through the inode prealloc list, and see if we have a
> preallocation that satisfies the allocation context:
>
> /* all fields in this condition don't change,
> * so we can skip locking for them */
> if (ac->ac_o_ex.fe_logical < pa->pa_lstart ||
> ac->ac_o_ex.fe_logical >= pa->pa_lstart + pa->pa_len)
> continue;
>
> /* non-extent files can't have physical blocks past 2^32 */
> if (!(EXT4_I(ac->ac_inode)->i_flags & EXT4_EXTENTS_FL) &&
> pa->pa_pstart + pa->pa_len > EXT4_MAX_BLOCK_FILE_PHYS)
> continue;
>
> /* found preallocated blocks, use them */
> spin_lock(&pa->pa_lock);
> if (pa->pa_deleted == 0 && pa->pa_free) {
>
> => Now we're good, and have an AC that satisfies us.
> => We call ext4_mb_use_inode_pa(ac, pa);
>
>
> But ext4_mb_use_inode_pa() has this:
>
> BUG_ON(pa->pa_free < len);
>
> Nowhere do we check the 'pa_free' value to decide if this preallocation is
> okay to use.
>
the 'len' value above is derived out of what we have in prealloc space.
ie, we do this
start = pa->pa_pstart + (ac->ac_o_ex.fe_logical - pa->pa_lstart);
end = min(pa->pa_pstart + pa->pa_len, start + ac->ac_o_ex.fe_len);
len = end - start;
Now to decide whether we need to use a particular inode prealloc space
we look at the pa->pa_lstart which is the start logical block number
mapping this prealloc space. So if the requested logical block number
falls within a prealloc space (ie within pa->pa_lstart , pa->pa_lstart + pa->pa_len)
we use the prealloc space. Done by the below conditional ext4_mb_use_preallocated
/* all fields in this condition don't change,
* so we can skip locking for them */
if (ac->ac_o_ex.fe_logical < pa->pa_lstart ||
ac->ac_o_ex.fe_logical >= pa->pa_lstart + pa->pa_len)
continue;
>
> Further down in ext4_mb_use_preallocated() we check the locality group
> prealloc list; for this, we DO check pa_free:
>
> spin_lock(&pa->pa_lock);
> if (pa->pa_deleted == 0 &&
> pa->pa_free >= ac->ac_o_ex.fe_len) {
>
> cpa = ext4_mb_check_group_pa(goal_block,
> pa, cpa);
>
locality group prealloc space is not looked with the logical block number.
We just claim need blocks from the prealloc space. Hence we check for the
available free blocks and the needed free blocks.
> So my question is: Is it a bug that we don't check that an inode
> preallocation has enough free blocks for the AC before we try to use it? I
> have hit the BUG_ON above at least once in my testing, but I can't
> characterize what the workload was at the time (nor can I reproduce it...).
>
You should not hit that. That would mean prealloc space accounting went wrong.
Which is really a BUG
-aneesh
Hi Aneesh:
Thanks for responding.
On Tue, Oct 13, 2009 at 10:23 PM, Aneesh Kumar K.V
<[email protected]> wrote:
> On Tue, Oct 13, 2009 at 11:06:35AM -0700, Curt Wohlgemuth wrote:
>> Hi all:
>>
>> I'm looking in ext4_mb_use_preallocated() and am seeing something odd.
>>
>> First we look through the inode prealloc list, and see if we have a
>> preallocation that satisfies the allocation context:
>>
>> ? ? ? ?/* all fields in this condition don't change,
>> ? ? ? ? * so we can skip locking for them */
>> ? ? ? ?if (ac->ac_o_ex.fe_logical < pa->pa_lstart ||
>> ? ? ? ? ? ? ? ?ac->ac_o_ex.fe_logical >= pa->pa_lstart + pa->pa_len)
>> ? ? ? ? ? ? ? ?continue;
>>
>> ? ? ? ?/* non-extent files can't have physical blocks past 2^32 */
>> ? ? ? ?if (!(EXT4_I(ac->ac_inode)->i_flags & EXT4_EXTENTS_FL) &&
>> ? ? ? ? ? ? ? ?pa->pa_pstart + pa->pa_len > EXT4_MAX_BLOCK_FILE_PHYS)
>> ? ? ? ? ? ? ? ?continue;
>>
>> ? ? ? ?/* found preallocated blocks, use them */
>> ? ? ? ?spin_lock(&pa->pa_lock);
>> ? ? ? ?if (pa->pa_deleted == 0 && pa->pa_free) {
>>
>> ? ? ? ? ? ? => Now we're good, and have an AC that satisfies us.
>> ? ? ? ? ? ? => We call ext4_mb_use_inode_pa(ac, pa);
>>
>>
>> But ext4_mb_use_inode_pa() has this:
>>
>> ? ? ? BUG_ON(pa->pa_free < len);
>>
>> Nowhere do we check the 'pa_free' value to decide if this preallocation is
>> okay to use.
>>
>
> the 'len' value above is derived out of what we have in prealloc space.
> ie, we do this
>
> ? ? start = pa->pa_pstart + (ac->ac_o_ex.fe_logical - pa->pa_lstart);
> ? ? end = min(pa->pa_pstart + pa->pa_len, start + ac->ac_o_ex.fe_len);
> ? ? len = end - start;
>
> Now to decide whether we need to use a particular inode prealloc space
> we look at the pa->pa_lstart which is the start logical block number
> mapping this prealloc space. So if the requested logical block number
> falls within a prealloc space (ie within pa->pa_lstart , pa->pa_lstart + pa->pa_len)
> we use the prealloc space. Done by the below conditional ext4_mb_use_preallocated
>
> ? ? ?/* all fields in this condition don't change,
> ? ? ? * so we can skip locking for them */
> ? ? ? if (ac->ac_o_ex.fe_logical < pa->pa_lstart ||
> ? ? ? ? ? ? ac->ac_o_ex.fe_logical >= pa->pa_lstart + pa->pa_len)
> ? ? ? ? ? ? ? ? ? ? ? ?continue;
>
>
>
>>
>> Further down in ext4_mb_use_preallocated() we check the locality group
>> prealloc list; for this, we DO check pa_free:
>>
>> ? ? ? ? ?spin_lock(&pa->pa_lock);
>> ? ? ? ? ?if (pa->pa_deleted == 0 &&
>> ? ? ? ? ? ? ? ? ? ? ? ? ?pa->pa_free >= ac->ac_o_ex.fe_len) {
>>
>> ? ? ? ? ? ? ? ? ?cpa = ext4_mb_check_group_pa(goal_block,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?pa, cpa);
>>
>
>
> locality group prealloc space is not looked with the logical block number.
> We just claim need blocks from the prealloc space. Hence we check for the
> available free blocks and the needed free blocks.
>
>
>
>> So my question is: ?Is it a bug that we don't check that an inode
>> preallocation has enough free blocks for the AC before we try to use it? ?I
>> have hit the BUG_ON above at least once in my testing, but I can't
>> characterize what the workload was at the time (nor can I reproduce it...).
>>
>
> You should not hit that. That would mean prealloc space accounting went wrong.
> Which is really a BUG
Okay, I get it.
In theory, it really does seem like a problem to me that we don't
check pa_free before we decide that the inode prealloc space is
suitable for this AC. In practice, though, this would only be an
issue if we got overlapping requests in the same logical block range,
which just shouldn't happen.
Looking further, it seems that the BUG_ON in ext4_mb_use_inode_pa()
fired for me after a lot of device failures, along with failures to
read the block bitmap and the like. So I suppose that trying to deal
gracefully with this sort of device error is just difficult to do, eh?
Thanks,
Curt
>
> -aneesh
>