Skip site navigation (1)Skip section navigation (2)
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>