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

[Brian Foster <bfoster@redhat.com>]

On Thu, Apr 23, 2020 at 02:52:39PM -0500, Eric Sandeen wrote:
> On 4/23/20 2:34 PM, Eric Sandeen wrote:
> > This scenario is the source of much confusion for admins and
> > support folks alike:
> > 
> > # touch mnt/newfile
> > touch: cannot touch ‘mnt/newfile’: No space left on device
> > # df -h mnt
> > Filesystem      Size  Used Avail Use% Mounted on
> > /dev/loop0      196M  137M   59M  71% /tmp/mnt
> > # df -i mnt/
> > Filesystem     Inodes IUsed IFree IUse% Mounted on
> > /dev/loop0     102400 64256 38144   63% /tmp/mnt
> > 
> > because it appears that there is plenty of space available, yet ENOSPC
> > is returned.
> > 
> > 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
> 
> > Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> > ---
> > 
> > Right now this depends on my "printk_once" patch but you can change
> > xfs_warn_once to xfs_warn or xfs_warn_ratelimited for testing.
> > 
> > Perhaps a 2nd patch to log a similar message if alignment failed due to
> > /contiguous/ free space constraints would be good as well?
> > 
> > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> > index 203e74fa64aa..10f32797e5ca 100644
> > --- a/fs/xfs/libxfs/xfs_alloc.c
> > +++ b/fs/xfs/libxfs/xfs_alloc.c
> > @@ -2303,8 +2303,12 @@ xfs_alloc_space_available(
> >  	/* do we have enough contiguous free space for the allocation? */
> >  	alloc_len = args->minlen + (args->alignment - 1) + args->minalignslop;
> >  	longest = xfs_alloc_longest_free_extent(pag, min_free, reservation);
> > -	if (longest < alloc_len)
> > +	if (longest < alloc_len) {
> > +		/* Did we fail only due to alignment? */
> > +		if (longest >= args->minlen)
> > +			args->alignfail = 1;
> >  		return false;
> > +	}
> >  
> >  	/*
> >  	 * Do we have enough free space remaining for the allocation? Don't
> > @@ -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..

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.

> >  	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)..?

Brian

> >  	return 0;
> >  error0:
> >  	xfs_perag_put(args->pag);
> > diff --git a/fs/xfs/libxfs/xfs_alloc.h b/fs/xfs/libxfs/xfs_alloc.h
> > index a851bf77f17b..29d13cd5c9ac 100644
> > --- a/fs/xfs/libxfs/xfs_alloc.h
> > +++ b/fs/xfs/libxfs/xfs_alloc.h
> > @@ -73,6 +73,7 @@ typedef struct xfs_alloc_arg {
> >  	int		datatype;	/* mask defining data type treatment */
> >  	char		wasdel;		/* set if allocation was prev delayed */
> >  	char		wasfromfl;	/* set if allocation is from freelist */
> > +	char		alignfail;	/* set if alloc failed due to alignmt */
> >  	struct xfs_owner_info	oinfo;	/* owner of blocks being allocated */
> >  	enum xfs_ag_resv_type	resv;	/* block reservation to use */
> >  } xfs_alloc_arg_t;
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index fda13cd7add0..808060649cad 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -3563,6 +3563,7 @@ xfs_bmap_btalloc(
> >  	args.mp = mp;
> >  	args.fsbno = ap->blkno;
> >  	args.oinfo = XFS_RMAP_OINFO_SKIP_UPDATE;
> > +	args.alignfail = 0;
> >  
> >  	/* Trim the allocation back to the maximum an AG can fit. */
> >  	args.maxlen = min(ap->length, mp->m_ag_max_usable);
> > diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> > index 7fcf62b324b0..e98dcb8e65eb 100644
> > --- a/fs/xfs/libxfs/xfs_ialloc.c
> > +++ b/fs/xfs/libxfs/xfs_ialloc.c
> > @@ -685,6 +685,7 @@ xfs_ialloc_ag_alloc(
> >  		 * but not to use them in the actual exact allocation.
> >  		 */
> >  		args.alignment = 1;
> > +		args.alignfail = 0;
> >  		args.minalignslop = igeo->cluster_align - 1;
> >  
> >  		/* Allow space for the inode btree to split. */
> > 
>