Re: [PATCH RFC] xfs: log message when allocation fails due to alignment constraints

[Brian Foster <bfoster@redhat.com>]

On Fri, Apr 24, 2020 at 08:41:28AM -0500, Eric Sandeen wrote:
> 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?)
> 

That seems more user friendly to me. A new XFS_MOUNT_INODE_ENOSPC or
some such mount flag might be sufficient...

Brian

> Thanks,
> -Eric