On 4/24/20 7:40 AM, Brian Foster wrote:
> On Thu, Apr 23, 2020 at 02:52:39PM -0500, Eric Sandeen wrote:
>> On 4/23/20 2:34 PM, Eric Sandeen wrote:
...
>>> Track this case in the allocation args structure, and when an allocation
>>> fails due to alignment constraints, leave a clue in the kernel logs:
>>>
>>> XFS (loop0): Failed metadata allocation due to 4-block alignment constraint
>>
>> Welp, I always realize what's wrong with the patch right after I send it;
>> I think this reports the failure on each AG that fails, even if a later
>> AG succeeds so I need to get the result up to a higher level.
>>
>
> Hmm, yeah.. the inode chunk allocation code in particular can make
> multiple attempts at xfs_alloc_vextent() before the higher level
> operation ultimately fails.
>
>> Still, can see what people think of the idea in general?
>>
>
> Seems reasonable to me in general..
>
>> Thanks,
>> -Eric
>>
...
>>> @@ -3067,8 +3071,10 @@ xfs_alloc_vextent(
>>> agsize = mp->m_sb.sb_agblocks;
>>> if (args->maxlen > agsize)
>>> args->maxlen = agsize;
>>> - if (args->alignment == 0)
>>> + if (args->alignment == 0) {
>>> args->alignment = 1;
>>> + args->alignfail = 0;
>>> + }
>
> Any reason this is reinitialized only when the caller doesn't care about
> alignment? This seems more like something that should be reset on each
> allocation call..
Hm probably not :)
> BTW I'm also wondering if this is something that could be isolated to a
> single location by looking at perag state instead of plumbing the logic
> through the allocator args (which is already a mess). I guess we no
> longer have the allocator's perag reference once we're back in the inode
> allocation code, but the xfs_ialloc_ag_select() code does use a perag to
> filter out AGs without enough space. I wonder if that's enough to assume
> alignment is the problem if we attempt a chunk allocation and it
> ultimately fails..? We could also just consider looking at the perag
> again in xfs_dialloc() if the allocation fails, since it looks like we
> still have a reference there.
Thanks, I'll give all that some thought.
>>> ASSERT(XFS_FSB_TO_AGNO(mp, args->fsbno) < mp->m_sb.sb_agcount);
>>> ASSERT(XFS_FSB_TO_AGBNO(mp, args->fsbno) < agsize);
>>> ASSERT(args->minlen <= args->maxlen);
>>> @@ -3227,6 +3233,13 @@ xfs_alloc_vextent(
>>>
>>> }
>>> xfs_perag_put(args->pag);
>>> + if (!args->agbp && args->alignment > 1 && args->alignfail) {
>>> + xfs_warn_once(args->mp,
>>> +"Failed %s allocation due to %u-block alignment constraint",
>>> + XFS_RMAP_NON_INODE_OWNER(args->oinfo.oi_owner) ?
>>> + "metadata" : "data",
>>> + args->alignment);
>>> + }
>
> Perhaps this should be ratelimited vs. printed once? I suppose there's
> not much value in continuing to print it once an fs is in this inode
> -ENOSPC state, but the tradeoff is that if the user clears the state and
> maybe runs into it again sometime later without a restart, they might
> not see the message and think it's something else. (What about hitting
> the same issue across multiple mounts, btw?). I suppose the ideal
> behavior would be to print once and never again until an inode chunk has
> been successfully allocated (or the system reset)..?
Yeah, I wasn't sure about this being a one-shot.
(Actually, it crossed my mind that maybe we could make the _once variant
reference something in the xfs_mount, so a one-shot warning would printk
once per mp, per mount session?)
Thanks,
-Eric
