Re: kern/47231 ffs (with or without WAPBL) does not preserve referential integrity of file data and corrupts files when crashing

["Konrad Schroder via gnats" <gnats-admin%NetBSD.org@localhost>]

The following reply was made to PR kern/47231; it has been noted by GNATS.

From: Konrad Schroder <perseant%hhhh.org@localhost>
To: gnats-bugs%NetBSD.org@localhost, jdolecek%netbsd.org@localhost, jakllsch%kollasch.net@localhost
Cc: 
Subject: Re: kern/47231 ffs (with or without WAPBL) does not preserve
 referential integrity of file data and corrupts files when crashing
Date: Thu, 31 Jul 2025 22:07:53 -0700

 I don't have a good solution to the general problem that FFS metadata 
 updates race the data to disk; but I can offer a potential solution to 
 the narrower issue of files becoming corrupted, rather than protected, 
 by an open-write-close-rename sequence.
 
 hannken@ mentioned fsync-on-close, but that would make WAPBL unbearably 
 slow even for ordinary file writes.  We could largely avoid that, 
 however, by instead treating the *rename* operation.  Compared to close, 
 rename is relatively less common, so the filesystem would not be as 
 badly affected in general, though of course this will depend on the 
 workload.  The patch is simple:
 
 --------8<--------
 Index: sys/ufs/ffs/ffs_vnops.c
 ===================================================================
 RCS file: /cvsroot/src/sys/ufs/ffs/ffs_vnops.c,v
 retrieving revision 1.138
 diff -u -r1.138 ffs_vnops.c
 --- sys/ufs/ffs/ffs_vnops.c     14 Dec 2021 11:06:12 -0000 1.138
 +++ sys/ufs/ffs/ffs_vnops.c     30 Jul 2025 21:42:13 -0000
 @@ -127,7 +127,7 @@
          { &vop_seek_desc, genfs_seek },                 /* seek */
          { &vop_remove_desc, ufs_remove },               /* remove */
          { &vop_link_desc, ufs_link },                   /* link */
 -       { &vop_rename_desc, ufs_rename },               /* rename */
 +       { &vop_rename_desc, ffs_rename },               /* rename */
          { &vop_mkdir_desc, ufs_mkdir },                 /* mkdir */
          { &vop_rmdir_desc, ufs_rmdir },                 /* rmdir */
          { &vop_symlink_desc, ufs_symlink },             /* symlink */
 @@ -576,3 +576,40 @@
  Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  *eobp = ffs_blkroundup(fs, size);
  Â Â Â Â Â Â Â  }
  Â }
 +
 +/*
 + * If we are using WAPBL, flush pages before a rename to protect the
 + * open-write-close-rename sequence, commonly used to "atomically"
 + * exchange the contents of an existing file.
 + */
 +int
 +ffs_rename(void *v)
 +{
 +#ifdef WAPBL
 +Â Â Â Â Â Â  struct vop_rename_args /* {
 +Â Â Â Â Â Â Â Â Â Â Â Â Â Â  struct vnode *a_fdvp;
 +Â Â Â Â Â Â Â Â Â Â Â Â Â Â  struct vnode *a_fvp;
 +Â Â Â Â Â Â Â Â Â Â Â Â Â Â  struct componentname *a_fcnp;
 +Â Â Â Â Â Â Â Â Â Â Â Â Â Â  struct vnode *a_tdvp;
 +Â Â Â Â Â Â Â Â Â Â Â Â Â Â  struct vnode *a_tvp;
 +Â Â Â Â Â Â Â Â Â Â Â Â Â Â  struct componentname *a_tcnp;
 +Â Â Â Â Â Â  } */ *ap = v;
 +Â Â Â Â Â Â  struct vnode *fvp = ap->a_fvp;
 +Â Â Â Â Â Â  int error;
 +
 +Â Â Â Â Â Â  KASSERT(fvp != NULL);
 +Â Â Â Â Â Â  if (fvp->v_type == VREG && fvp->v_mount->mnt_wapbl) {
 +Â Â Â Â Â Â Â Â Â Â Â Â Â Â  error = vn_lock(fvp, LK_EXCLUSIVE);
 +Â Â Â Â Â Â Â Â Â Â Â Â Â Â  if (error)
 +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  return error;
 +Â Â Â Â Â Â Â Â Â Â Â Â Â Â  rw_enter(fvp->v_uobj.vmobjlock, RW_WRITER);
 +Â Â Â Â Â Â Â Â Â Â Â Â Â Â  error = VOP_PUTPAGES(fvp, 0, 0,
 +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  PGO_CLEANIT | PGO_SYNCIO | PGO_ALLPAGES);
 +Â Â Â Â Â Â Â Â Â Â Â Â Â Â  VOP_UNLOCK(fvp);
 +Â Â Â Â Â Â Â Â Â Â Â Â Â Â  if (error)
 +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  return error;
 +Â Â Â Â Â Â  }
 +#endif /* WAPBL */
 +
 +Â Â Â Â Â Â  return ufs_rename(v);
 +}
 Index: sys/ufs/ffs/ffs_extern.h
 ===================================================================
 RCS file: /cvsroot/src/sys/ufs/ffs/ffs_extern.h,v
 retrieving revision 1.87.2.1
 diff -u -r1.87.2.1 ffs_extern.h
 --- sys/ufs/ffs/ffs_extern.h    13 May 2023 11:51:14 -0000 1.87.2.1
 +++ sys/ufs/ffs/ffs_extern.h    30 Jul 2025 21:42:13 -0000
 @@ -139,6 +139,7 @@
   int    ffs_unlock(void *);
   int    ffs_islocked(void *);
   int    ffs_full_fsync(struct vnode *, int);
 +int    ffs_rename(void *);
 
  Â /* ffs_extattr.c */
   int    ffs_openextattr(void *);
 --------8<--------
 
 The performance hit is apparent when unpacking an archive over existing 
 files:
 
  Â  unpatched# tar xf pkgsrc.tar
    27.9u 173.4s 5:17.68 63.4% 0+0k 30296+69705io 12pf+0w    [1]
  Â  unpatched# tar xf pkgsrc.tar
    45.2u 295.9s 8:10.32 69.5% 0+0k 81763+55838io 8pf+0w     [2]
 
  Â  patched# time tar xf pkgsrc.tar
    27.720u 167.187s 5:31.39 58.8%  0+0k 30389+68562io 16pf+0w      
 (+4%real time vs [1])
  Â  patched# time tar xf pkgsrc.tar
    47.442u 333.776s 20:47.00 30.5% 0+0k 82104+56157io 8pf+0w       
 (+154% vs [2])
 
 (and of course for this specific case is avoidable:
 
  Â  patched# time tar -U -x -f pkgsrc.tar
    27.921u 220.225s 7:12.98 57.3%  0+0k 75772+55772io 23pf+0w      (+5% 
 vs [2]))
 
 but compared to not using WAPBL at all, it's not so bad:
 
  Â  no_wapbl# tar xf pkgsrc.tar
    30.2u 201.8s 9:21.39 41.3% 0+0k 29162+675598io 1pf+0w           (+77% 
 vs [1])
  Â  no_wapbl# tar xf pkgsrc.tar
    47.1u 339.9s 17:55.27 35.9% 0+0k 81971+1365043io 8pf+0w         
 (+119% vs [2])
 
 I expect that for most real-world use it will be a net improvement over 
 mounting without WAPBL.
 
 Thoughts?
 
 Thanks,
 
 --
 Konrad Schroder
 perseant%NetBSD.org@localhost