Date: Mon, 03 Dec 2007 08:10:21 +1000 From: David Cecil <david.cecil@nokia.com> To: ext Bruce Evans <brde@optusnet.com.au> Cc: freebsd-fs@freebsd.org, Don Lewis <truckman@freebsd.org> Subject: Re: File remove problem Message-ID: <47532D4D.4020300@nokia.com> In-Reply-To: <20071202193924.P1745@besplex.bde.org> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <ufs/ufs/gjournal.h>
>>> #endif
>>> +#include <ufs/ffs/fs.h>
>>>
>>> /*
>>> * 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" <sam@errno.com>
> % ----------------------------
> % % 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 <sys/cdefs.h>
> % -__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"
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?47532D4D.4020300>