From owner-freebsd-fs@FreeBSD.ORG Sun Dec 2 22:10:34 2007 Return-Path: Delivered-To: freebsd-fs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 16A7B16A418 for ; Sun, 2 Dec 2007 22:10:34 +0000 (UTC) (envelope-from david.cecil@nokia.com) Received: from mgw-mx03.nokia.com (smtp.nokia.com [192.100.122.230]) by mx1.freebsd.org (Postfix) with ESMTP id 7C6C013C478 for ; Sun, 2 Dec 2007 22:10:33 +0000 (UTC) (envelope-from david.cecil@nokia.com) Received: from esebh106.NOE.Nokia.com (esebh106.ntc.nokia.com [172.21.138.213]) by mgw-mx03.nokia.com (Switch-3.2.6/Switch-3.2.6) with ESMTP id lB2MAEbh009445; Mon, 3 Dec 2007 00:10:31 +0200 Received: from esebh103.NOE.Nokia.com ([172.21.143.33]) by esebh106.NOE.Nokia.com with Microsoft SMTPSVC(6.0.3790.1830); Mon, 3 Dec 2007 00:10:26 +0200 Received: from syebe101.NOE.Nokia.com ([172.30.128.65]) by esebh103.NOE.Nokia.com with Microsoft SMTPSVC(6.0.3790.1830); Mon, 3 Dec 2007 00:10:26 +0200 Received: from [172.30.67.30] ([172.30.67.30]) by syebe101.NOE.Nokia.com with Microsoft SMTPSVC(6.0.3790.1830); Mon, 3 Dec 2007 09:10:21 +1100 Message-ID: <47532D4D.4020300@nokia.com> Date: Mon, 03 Dec 2007 08:10:21 +1000 From: David Cecil User-Agent: Thunderbird 1.5.0.12 (Windows/20070509) MIME-Version: 1.0 To: ext Bruce Evans References: <20071201215706.B12006@besplex.bde.org> <200712012207.lB1M7oNg015468@gw.catspoiler.org> <20071202055919.GR83121@deviant.kiev.zoral.com.ua> <20071202183809.I1560@besplex.bde.org> <20071202193924.P1745@besplex.bde.org> In-Reply-To: <20071202193924.P1745@besplex.bde.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-OriginalArrivalTime: 02 Dec 2007 22:10:21.0712 (UTC) FILETIME=[2184E900:01C83530] X-Nokia-AV: Clean Cc: freebsd-fs@freebsd.org, Don Lewis Subject: Re: File remove problem X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 02 Dec 2007 22:10:34 -0000 What's the plan with these patches guys? Are they likely to be committed to current? I guess it's getting late in the game to commit to the 7.0 branch. Sorry for the resend Bruce. Thanks, Dave ext Bruce Evans wrote: > A second reply. Sorry for so many. > > On Sun, 2 Dec 2007, Bruce Evans wrote: > >> On Sun, 2 Dec 2007, Kostik Belousov wrote: > >>> I would argue that the ufs already knows too much about the ffs. But, >>> this seems to be the first explicit reference to the ffs from the ufs >>> code. With your approval, see below. > >>> diff --git a/sys/ufs/ufs/ufs_inode.c b/sys/ufs/ufs/ufs_inode.c >>> index 448f436..22e29e9 100644 >>> --- a/sys/ufs/ufs/ufs_inode.c >>> +++ b/sys/ufs/ufs/ufs_inode.c >>> @@ -60,6 +60,7 @@ __FBSDID("$FreeBSD: src/sys/ufs/ufs/ufs_inode.c,v >>> 1.69 2007/06/22 13:22:37 kib E >>> #ifdef UFS_GJOURNAL >>> #include >>> #endif >>> +#include >>> >>> /* >>> * Last reference to an inode. If necessary, write or delete it. >> >> ufs/ffs includes are conventionally separated from ufs/ufs includes by a >> blank line. About 2/3 of the files in ufs/ffs follow this convention. >> >>> @@ -90,8 +91,7 @@ ufs_inactive(ap) >>> ufs_gjournal_close(vp); >>> #endif >>> if ((ip->i_effnlink == 0 && DOINGSOFTDEP(vp)) || >>> - (ip->i_nlink <= 0 && >>> - (vp->v_mount->mnt_flag & MNT_RDONLY) == 0)) { >>> + (ip->i_nlink <= 0 && !VFSTOUFS(mp)->um_fs->fs_ronly)) { >>> loop: >>> if (vn_start_secondary_write(vp, &mp, V_NOWAIT) != 0) { >>> /* Cannot delete file while file system is suspended */ >>> @@ -121,7 +121,7 @@ ufs_inactive(ap) >>> } >>> if (ip->i_effnlink == 0 && DOINGSOFTDEP(vp)) >>> softdep_releasefile(ip); >>> - if (ip->i_nlink <= 0 && (vp->v_mount->mnt_flag & MNT_RDONLY) == >>> 0) { >>> + if (ip->i_nlink <= 0 && !VFSTOUFS(mp)->um_fs->fs_ronly) { >>> #ifdef QUOTA >>> if (!getinoquota(ip)) >>> (void)chkiq(ip, -1, NOCRED, FORCE); >>> >> >> Should be ip->i_fs->fs_ronly. >> >> The locking for fs_ronly is unclear. It seems to be locked mainly by >> vn_start_write(), and that enough for everything except probably access >> time changes. > > Actually. I hope that this MNT_RDONLY check can just go away. I now see > that it part of previous attempts to fix the bugs in this area. > > % RCS file: /home/ncvs/src/sys/ufs/ufs/ufs_inode.c,v > % Working file: ufs_inode.c > % head: 1.69 > % ---------------------------- > % revision 1.64 > % date: 2005/09/23 20:49:57; author: delphij; state: Exp; lines: +1 -1 > % Restore a historical ufs_inactive behavior that has been changed > % in rev. 1.40 of ufs_inode.c, which allows an inode being truncated > % even when the filesystem itself is marked RDONLY. A subsequent > % call of UFS_TRUNCATE (ffs_truncate) would panic the system as it > % asserts that it can only be called when the filesystem is mounted > % read-write (same changeset, rev. 1.74 of sys/ufs/ffs/ffs_inode.c). > % % Because ffs_mount() already takes care of sync'ing the filesystem > % to disk before being downgraded to readonly, it appears to be more > % desirable that we should not permit this sort of writes to disk. > % % This change would fix a panic that occours when read-only mounted > % a corrupted filesystem and doing some file operations. > % % MT6/5/4 candidate > % % Reviewed by: mckusick > % ---------------------------- > % ... > % ---------------------------- > % revision 1.40 > % date: 2002/01/15 07:17:12; author: mckusick; state: Exp; lines: > +2 -2 > % When downgrading a filesystem from read-write to read-only, operations > % involving file removal or file update were not always being fully > % committed to disk. The result was lost files or corrupted file data. > % This change ensures that the filesystem is properly synced to disk > % before the filesystem is down-graded. > % % This delta also fixes a long standing bug in which a file open for > % reading has been unlinked. When the last open reference to the file > % is closed, the inode is reclaimed by the filesystem. Previously, > % if the filesystem had been down-graded to read-only, the inode could > % not be reclaimed, and thus was lost and had to be later recovered > % by fsck. With this change, such files are found at the time of the > % down-grade. Normally they will result in the filesystem down-grade > % failing with `device busy'. If a forcible down-grade is done, then > % the affected files will be revoked causing the inode to be released > % and the open file descriptors to begin failing on attempts to read. > % % Submitted by: "Sam Leffler" > % ---------------------------- > % % Index: ufs_inode.c > % =================================================================== > % RCS file: /home/ncvs/src/sys/ufs/ufs/ufs_inode.c,v > % retrieving revision 1.63 > % retrieving revision 1.64 > % diff -u -2 -r1.63 -r1.64 > % --- ufs_inode.c 17 Mar 2005 11:58:43 -0000 1.63 > % +++ ufs_inode.c 23 Sep 2005 20:49:57 -0000 1.64 > % @@ -36,5 +36,5 @@ > % % #include > % -__FBSDID("$FreeBSD: src/sys/ufs/ufs/ufs_inode.c,v 1.63 2005/03/17 > 11:58:43 jeff Exp $"); > % +__FBSDID("$FreeBSD: src/sys/ufs/ufs/ufs_inode.c,v 1.64 2005/09/23 > 20:49:57 delphij Exp $"); > % % #include "opt_quota.h" > % @@ -84,5 +84,5 @@ > % if (ip->i_effnlink == 0 && DOINGSOFTDEP(vp)) > % softdep_releasefile(ip); > % - if (ip->i_nlink <= 0) { > % + if (ip->i_nlink <= 0 && (vp->v_mount->mnt_flag & MNT_RDONLY) == > 0) { > % (void) vn_write_suspend_wait(vp, NULL, V_WAIT); > % #ifdef QUOTA > > % Index: ufs_inode.c > % =================================================================== > % RCS file: /home/ncvs/src/sys/ufs/ufs/ufs_inode.c,v > % retrieving revision 1.39 > % retrieving revision 1.40 > % diff -u -2 -r1.39 -r1.40 > % --- ufs_inode.c 11 Oct 2001 17:52:20 -0000 1.39 > % +++ ufs_inode.c 15 Jan 2002 07:17:12 -0000 1.40 > % @@ -37,5 +37,5 @@ > % * > % * @(#)ufs_inode.c 8.9 (Berkeley) 5/14/95 > % - * $FreeBSD: src/sys/ufs/ufs/ufs_inode.c,v 1.39 2001/10/11 17:52:20 > jhb Exp $ > % + * $FreeBSD: src/sys/ufs/ufs/ufs_inode.c,v 1.40 2002/01/15 07:17:12 > mckusick Exp $ > % */ > % % @@ -85,5 +85,5 @@ > % if (ip->i_effnlink == 0 && DOINGSOFTDEP(vp)) > % softdep_releasefile(ip); > % - if (ip->i_nlink <= 0 && (vp->v_mount->mnt_flag & MNT_RDONLY) == > 0) { > % + if (ip->i_nlink <= 0) { > % (void) vn_write_suspend_wait(vp, NULL, V_WAIT); > % #ifdef QUOTA > > Rev.1.40 of ufs_inode.c goes with rev.1.182 of ufs_vnops.c and > rev.1.74 of > ffs_vnops.c to fix truncation of unlinked open files in mount-update. > Rev.1.64 breaks this case by backing out 1.40. I think 1.64 is an > attempt > to work around the other bugs. It breaks the case of unlinked open files > more deterministically, but this case is relatively uncommon. Again, I > was "lucky" to debug this partly under 5.2 which doesn't have 1.64, so > the extra (null?) truncations for closed files were relatively common. > > So it should be safe to remove all the r/o checks in ufs_inactive() after > fixing the other bugs. ffs_truncate alread panics if fs_ronly, but only > in some cases. In particular, it doesn't panic for truncations that > don't > change the file size. Such truncations aren't quite null, since > standards > require [f]truncate(2) to mark the ctime and mtime for update. > ffs_truncate() sets the marks, which is correct for null truncations from > userland but not ones from syncer internals. Setting of the marks when > fs_ronly is set should cause panics later (my patch has a vprint() for > it). > > Bruce > _______________________________________________ > freebsd-fs@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-fs > To unsubscribe, send any mail to "freebsd-fs-unsubscribe@freebsd.org" >