Date: Sat, 8 Aug 1998 18:01:00 -0400 (EDT) From: Brian Feldman <green@zone.syracuse.net> To: Kirk McKusick <mckusick@McKusick.COM> Cc: Julian Elischer <julian@whistle.com>, Luoqi Chen <luoqi@watermarkgroup.com>, kkennawa@physics.adelaide.edu.au, dg@root.com, current@FreeBSD.ORG Subject: Re: Softupdates panic (fwd) Message-ID: <Pine.BSF.4.02.9808081757560.17846-100000@zone.syracuse.net> In-Reply-To: <199808081852.LAA06087@flamingo.McKusick.COM>
next in thread | previous in thread | raw e-mail | index | archive | help
In the current ufs_vnops.c I have, the correct line 1146 is:
IFTODT(ip->i_mode), newparent ? newparent :
doingdirectory);
My version is $Id: ufs_vnops.c,v 1.97 1998/07/27 15:37:00 bde Exp $, I
guess we've got a non-committed version on our hands.
Cheers,
Brian Feldman
green@unixhelp.org
On Sat, 8 Aug 1998, Kirk McKusick wrote:
> The problem occured when renaming a directory to a new parent, but
> not when renaming a directory within the same parent (write a one
> line C program that does ``rename("d1", "d2");''). Luoqi Chen's
> fix (forwarded message below) for the broken case, breaks the same
> directory rename case. Thus, the soft update code needs to be aware of
> which case it is handling. I now pass this information through and do the
> right thing for both cases. Affected files are /sys/ufs/ufs/ufs_vnops.c
> and /sys/ufs/ffs/ffs_softdep.c. Note there is a related one line fix
> if ffs_softdep.c which sometimes caused the rename to fail with an
> EINVAL error (compounding the difficulty of trying to track down this
> bug). The diffs follow.
>
> Kirk McKusick
>
> Index: ffs_softdep.c
> ===================================================================
> RCS file: /master/sys/ufs/ffs/ffs_softdep.c,v
> retrieving revision 2.21
> diff -c -r2.21 ffs_softdep.c
> *** ffs_softdep.c 1998/06/12 17:54:33 2.21
> --- ffs_softdep.c 1998/08/08 18:32:36
> ***************
> *** 52,58 ****
> * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> * SUCH DAMAGE.
> *
> ! * @(#)ffs_softdep.c 9.27 (McKusick) 6/12/98
> */
>
> /*
> --- 52,58 ----
> * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> * SUCH DAMAGE.
> *
> ! * @(#)ffs_softdep.c 9.28 (McKusick) 8/8/98
> */
>
> /*
> ***************
> *** 2205,2210 ****
> --- 2205,2211 ----
> } else {
> dirrem = dap->da_previous;
> pagedep = dirrem->dm_pagedep;
> + dirrem->dm_dirinum = pagedep->pd_ino;
> add_to_worklist(&dirrem->dm_list);
> }
> if (inodedep_lookup(VFSTOUFS(pagedep->pd_mnt)->um_fs, dap->da_newinum,
> ***************
> *** 2384,2389 ****
> --- 2385,2404 ----
> */
> dirrem = newdirrem(bp, dp, ip, isrmdir);
> pagedep = dirrem->dm_pagedep;
> + /*
> + * The possible values for isrmdir:
> + * 0 - non-directory file rename
> + * 1 - directory rename within same directory
> + * inum - directory rename to new directory of given inode number
> + * When renaming to a new directory, we are both deleting and
> + * creating a new directory entry, so the link count on the new
> + * directory should not change. Thus we do not need the followup
> + * dirrem which is usually done in handle_workitem_remove. We set
> + * the DIRCHG flag to tell handle_workitem_remove to skip the
> + * followup dirrem.
> + */
> + if (isrmdir > 1)
> + dirrem->dm_state |= DIRCHG;
>
> /*
> * Whiteouts have no additional dependencies,
> ***************
> *** 2493,2498 ****
> --- 2508,2523 ----
> ip->i_flag |= IN_CHANGE;
> if ((error = VOP_TRUNCATE(vp, (off_t)0, 0, p->p_ucred, p)) != 0)
> softdep_error("handle_workitem_remove: truncate", error);
> + /*
> + * Rename a directory to a new parent. Since, we are both deleting
> + * and creating a new directory entry, the link count on the new
> + * directory should not change. Thus we skip the followup dirrem.
> + */
> + if (dirrem->dm_state & DIRCHG) {
> + vput(vp);
> + WORKITEM_FREE(dirrem, D_DIRREM);
> + return;
> + }
> ACQUIRE_LOCK(&lk);
> (void) inodedep_lookup(ip->i_fs, dirrem->dm_oldinum, DEPALLOC,
> &inodedep);
> Index: ufs_vnops.c
> ===================================================================
> RCS file: /master/sys/ufs/ufs/ufs_vnops.c,v
> retrieving revision 2.29
> diff -c -r2.29 ufs_vnops.c
> *** ufs_vnops.c 1998/06/08 18:52:11 2.29
> --- ufs_vnops.c 1998/08/08 18:20:28
> ***************
> *** 930,936 ****
> }
> ip->i_flag |= IN_RENAME;
> oldparent = dp->i_number;
> ! doingdirectory++;
> }
>
> /*
> --- 930,936 ----
> }
> ip->i_flag |= IN_RENAME;
> oldparent = dp->i_number;
> ! doingdirectory = 1;
> }
>
> /*
> ***************
> *** 1073,1079 ****
> goto bad;
> }
> if ((error = ufs_dirrewrite(dp, xp, ip->i_number,
> ! IFTODT(ip->i_mode), doingdirectory)) != 0)
> goto bad;
> if (doingdirectory) {
> if (!newparent) {
> --- 1073,1080 ----
> goto bad;
> }
> if ((error = ufs_dirrewrite(dp, xp, ip->i_number,
> ! IFTODT(ip->i_mode),
> ! newparent ? newparent : doingdirectory)) != 0)
> goto bad;
> if (doingdirectory) {
> if (!newparent) {
>
> ---------- Forwarded message ----------
> Date: Fri, 7 Aug 1998 12:09:03 -0400 (EDT)
> From: Luoqi Chen <luoqi@watermarkgroup.com>
> To: julian@whistle.com, kkennawa@physics.adelaide.edu.au
> Cc: current@FreeBSD.ORG
> Subject: Re: Softupdates panic
>
> > Well, here's the problem simplified:
> >
> > cd /tmp (assuming mounted soft-updates)
> > mkdir d1
> > mkdir d1/d2
> > mkdir d2
> > mv d2 d1
> > rmdir d1/d2
> > rmdir d1
> > [system will panic in 15 seconds at 'sync' of that data.]
> >
> > fix to follow. (and checked in I guess)
> >
> > julian
> >
> I looked at the problem and figured out what went wrong. During the rename
> step (mv d2 d1), the target directory d1/d2 is deleted and a dirrem
> dependency is generated. Normally, a dirrem dependency would decrease
> the parent's link count by 1 and subdirectory's link count by 2. But for
> this particular dirrem, we don't want to decrement the parent's link count,
> otherwise we would the panic above. The solution would be mark this dirrem
> as such, and don't decrement the parent's link count when it's handled.
> Attached is a fix.
>
> -lq
>
> Index: ffs_softdep.c
> ===================================================================
> RCS file: /fun/cvs/src/contrib/sys/softupdates/ffs_softdep.c,v
> retrieving revision 1.12
> diff -u -r1.12 ffs_softdep.c
> --- ffs_softdep.c 1998/06/12 21:21:26 1.12
> +++ ffs_softdep.c 1998/08/07 15:37:57
> @@ -2436,6 +2436,7 @@
> * Allocate a new dirrem and ACQUIRE_LOCK.
> */
> dirrem = newdirrem(bp, dp, ip, isrmdir);
> + dirrem->dm_state |= DIRCHG;
> pagedep = dirrem->dm_pagedep;
>
> /*
> @@ -2546,6 +2547,15 @@
> ip->i_flag |= IN_CHANGE;
> if ((error = UFS_TRUNCATE(vp, (off_t)0, 0, p->p_ucred, p)) != 0)
> softdep_error("handle_workitem_remove: truncate", error);
> + /*
> + * Target directory deletion during a directory rename. The
> + * parent directory's link count doesn't need to be decremented.
> + */
> + if (dirrem->dm_state & DIRCHG) {
> + vput(vp);
> + WORKITEM_FREE(dirrem, D_DIRREM);
> + return;
> + }
> ACQUIRE_LOCK(&lk);
> (void) inodedep_lookup(ip->i_fs, dirrem->dm_oldinum, DEPALLOC,
> &inodedep);
> -------- End Forwarded message --------
>
> To Unsubscribe: send mail to majordomo@FreeBSD.org
> with "unsubscribe freebsd-current" in the body of the message
>
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-current" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.BSF.4.02.9808081757560.17846-100000>