| Message ID | 37a73948-ff5a-5375-c2e7-54174ae75462@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] xfs: log message when allocation fails due to alignment constraints | expand |
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. Still, can see what people think of the idea 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; > + } > 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); > + } > 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. */ >
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. */ > > >
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
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
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; + } 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); + } 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. */
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 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?