From owner-freebsd-current Sat Aug 8 15:02:11 1998 Return-Path: Received: (from majordom@localhost) by hub.freebsd.org (8.8.8/8.8.8) id PAA29654 for freebsd-current-outgoing; Sat, 8 Aug 1998 15:02:11 -0700 (PDT) (envelope-from owner-freebsd-current@FreeBSD.ORG) Received: from zone.syracuse.net (zone.syracuse.net [205.232.47.17]) by hub.freebsd.org (8.8.8/8.8.8) with ESMTP id PAA29649 for ; Sat, 8 Aug 1998 15:02:08 -0700 (PDT) (envelope-from green@zone.syracuse.net) Received: from localhost (green@localhost) by zone.syracuse.net (8.8.8/8.8.7) with SMTP id SAA17895; Sat, 8 Aug 1998 18:01:00 -0400 (EDT) Date: Sat, 8 Aug 1998 18:01:00 -0400 (EDT) From: Brian Feldman To: Kirk McKusick cc: Julian Elischer , Luoqi Chen , kkennawa@physics.adelaide.edu.au, dg@root.com, current@FreeBSD.ORG Subject: Re: Softupdates panic (fwd) In-Reply-To: <199808081852.LAA06087@flamingo.McKusick.COM> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-current@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG 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 > 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