Re: [PATCH v2] xfs: prevent close() from hanging on frozen filesystems

[Aditya Prakash Srivastava]

Hi Christoph,

Thank you for the detailed review and feedback.

The procedural guidance is helpful. I'll split the changes into
separate patches, move the reproducer and additional background into
the cover letter, and resend as a standalone patch series.

I'll incorporate the comments on the EOFBLOCKS_RELEASED handling,
transaction flag naming, comment formatting, and the transaction
allocation path in the next revision.

Regarding xfstests, I plan to convert it into an xfstests regression
test in the near future so this scenario can be validated automatically
going forward.

Thanks again for taking the time to review this.

Regards,
Aditya Prakash Srivastava

On Fri, Jun 12, 2026 at 11:20 AM Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
>
> Hi Aditya,
>
> this looks mosltly good.  A few procedural comments first, I'll then
> move on to the code nitpicks.
>
>  - please don't respond to the previous patch, always post standalone.
>    git₋send-email gets this right.
>  - a lot of the history in this commit log belongs into a cover letter,
>    not the commit log itself.  Same for the reproducer.
>  - please split this up into multiple patches dong one thing a a time,
>    e.g.
>
> xfs: add a XFS_TRANS_TRYLOCK flag
> xfs: prevent close() from hanging on frozen filesystems
>
> > A simple GPLv2-licensed C reproducer demonstrating the hang (compile
> > with -pthread):
>
> Can you also wire this up to xfstests?
>
> > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> > index 0ab00615f1ad..56c7919bf1e5 100644
> > --- a/fs/xfs/xfs_bmap_util.c
> > +++ b/fs/xfs/xfs_bmap_util.c
> > @@ -574,7 +574,8 @@ xfs_can_free_eofblocks(
> >   */
> >  int
> >  xfs_free_eofblocks(
> > -     struct xfs_inode        *ip)
> > +     struct xfs_inode        *ip,
> > +     uint                    flags)
>
> Maybe name this trans_flags so that the usage sticks out?
>
> > @@ -1807,8 +1807,17 @@ xfs_file_release(
> >       if (!xfs_iflags_test(ip, XFS_EOFBLOCKS_RELEASED) &&
> >           xfs_ilock_nowait(ip, XFS_IOLOCK_EXCL)) {
> >               if (xfs_can_free_eofblocks(ip) &&
> > -                 !xfs_iflags_test_and_set(ip, XFS_EOFBLOCKS_RELEASED))
> > -                     xfs_free_eofblocks(ip);
> > +                 !xfs_iflags_test_and_set(ip, XFS_EOFBLOCKS_RELEASED)) {
> > +                     int error = xfs_free_eofblocks(ip, XFS_TRANS_TRYLOCK);
> > +
> > +                     /*
> > +                      * If transaction allocation fails due to a frozen/freezing
> > +                      * filesystem, clear the released flag so that subsequent
> > +                      * releases or background blockgc can retry post-thaw.
> > +                      */
> > +                     if (error == -EAGAIN)
> > +                             xfs_iflags_clear(ip, XFS_EOFBLOCKS_RELEASED);
>
> The comment has two overly long lines.
>
> Also the resetting the flag on error is a bit odd.  As far as I can tell
> there is no need to clear it before the action, so I think we can just
> move to clearing it only on successful return, e.g.:
>
>         if (!xfs_iflags_test(ip, XFS_EOFBLOCKS_RELEASED) &&
>             xfs_ilock_nowait(ip, XFS_IOLOCK_EXCL)) {
>                 if (!xfs_iflags_test(ip, XFS_EOFBLOCKS_RELEASED) &&
>                     xfs_can_free_eofblocks(ip) &&
>                     !xfs_free_eofblocks(ip, XFS_TRANS_TRYLOCK))
>                         xfs_iflags_set(ip, XFS_EOFBLOCKS_RELEASED);
>
>
> > +     if (!(flags & XFS_TRANS_NO_WRITECOUNT)) {
> > +             /*
> > +              * If TRYLOCK is specified, attempt a non-blocking lock to
> > +              * avoid blocking indefinitely on frozen/freezing filesystems.
> > +              */
>
> This would be a bit cleaner as:
>
>         if (flags & XFS_TRANS_TRYLOCK) {
>                 if (!sb_start_intwrite_trylock(mp->m_super)) {
>                         kmem_cache_free(xfs_trans_cache, tp);
>                         return ERR_PTR(-EAGAIN);
>                 }
>         } else if (!(flags & XFS_TRANS_NO_WRITECOUNT)) {
>                 sb_start_intwrite(mp->m_super);
>         }
>
> And maybe the flag name would better match XFS_TRANS_NO_WRITECOUNT
> a bit, e.g. XFS_TRANS_WRITECOUNT_TRYLOCK?
>
> Maybe also add an assert that XFS_TRANS_NO_WRITECOUNT and
> XFS_TRANS_NO_WRITECOUNT are not specified at the same time.
>