Date: Sun, 2 Dec 2007 20:07:07 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Bruce Evans <brde@optusnet.com.au> Cc: freebsd-fs@freebsd.org, Don Lewis <truckman@freebsd.org> Subject: Re: File remove problem Message-ID: <20071202193924.P1745@besplex.bde.org> In-Reply-To: <20071202183809.I1560@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>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20071202193924.P1745>