Date: Fri, 30 Nov 2007 11:38:47 +1000 From: David Cecil <david.cecil@nokia.com> To: ext Bruce Evans <brde@optusnet.com.au> Cc: freebsd-fs@freebsd.org Subject: Re: File remove problem Message-ID: <474F69A7.9090404@nokia.com> In-Reply-To: <20071130112043.H7217@besplex.bde.org> References: <474F4E46.8030109@nokia.com> <20071130112043.H7217@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Thanks Bruce.
Actually, I had found the same problem, and I came up with the first
line of your patch (adding IN_MODIFIED) myself, but I still saw the
problem. I didn't pick up on the need for the second line (else if
(DOINGASYNC(dvp)) {) though. It's a default mount, so I don't
understand how that will help, i.e. it won't be an async mount, right?
One more point to address Julian's question, the partition is not
mounted with soft updates.
Thanks,
Dave
ext Bruce Evans wrote:
> On Fri, 30 Nov 2007, David Cecil wrote:
>
>> I've been looking into a problem we're seeing on FreeBSD 6.1, though
>> I believe the bug will exist in current, or at least 7.0.
>>
>> Under certain circumstances, when a file is removed from the
>> filesystem, and the filesystem is remounted read-only immediately
>> afterwards, an error such as the following is displayed:
>> g_vfs_done():mirror/gmroots1f[WRITE(offset=1349058560,
>> length=16384)]error = 1
>>
>> I have determined that the buffer contains an update to the inode for
>> the file that's deleted. The inode for the directory change appears
>> to be updated correctly. So what's not making it to disk is the
>> updated file inode with its changed link counts (should now be
>> zero). So, somehow this inode is being missed during the sync prior
>> to the remount completing.
>
> I think I understand this now. Try this fix (only the first half):
>
> % Index: ufs/ufs/ufs_lookup.c
> % ===================================================================
> % RCS file: /home/ncvs/src/sys/ufs/ufs/ufs_lookup.c,v
> % retrieving revision 1.70
> % diff -u -2 -r1.70 ufs_lookup.c
> % --- ufs/ufs/ufs_lookup.c 7 Apr 2004 03:47:20 -0000 1.70
> % +++ ufs/ufs/ufs_lookup.c 29 Sep 2007 11:20:03 -0000
> % @@ -1053,9 +1054,9 @@
> % ip->i_nlink--;
> % DIP(ip, i_nlink) = ip->i_nlink;
> % - ip->i_flag |= IN_CHANGE;
> % + ip->i_flag |= IN_CHANGE | IN_MODIFIED;
> % }
> % if (flags & DOWHITEOUT)
> % error = bwrite(bp);
> % - else if (DOINGASYNC(dvp) && dp->i_count != 0) {
> % + else if (DOINGASYNC(dvp)) {
> % bdwrite(bp);
> % error = 0;
>
> IN_MODIFIED should always be set when the inode is changed, but many
> places are sloppy and only set IN_CHANGE, relying on IN_MODIFIED being
> set later when the update mark IN_CHANGE is converted to an actual
> change of the ctime. Normally this is just an obfuscation, but in the
> case of a r/w to a r/o transition, the update mark is discarded in
> ufs_itimes(). The related complication that ffs_update() doesn't trust
> IN_MODIFIED in the waitfor != 0 case apparently doesn't help. This
> complication is needed (modulo bugs like the one fixed above) only for
> soft updates, and is used mainly when ffs_fsync() passes waitfor !=
> 0. The r/w to r/o transition should be doing a waitfor != 0 (spelled
> MNT_WAIT) sync, so I don't quite understand why the complication doesn't
> help.
>
> Note that there are 2 r/o flags, MNT_RDONLY in the (re)mount flags and
> fs_ronly in the superblock. During the transition, MNT_RDONLY is set,
> so ufs_itimes() discards IN_CHANGE, while fs_ronly remains clear so
> that the final inode update is allowed. The transition requires even
> more delicate handling for IN_ACCESS.
>
> There are many other places in which IN_MODIFIED is not set correctly,
> but the above should be enough to fix unlink().
>
> I have used a version of the above for many years but just added back
> the IN_CHANGE setting in it to minimise this change.
>
> Bruce
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?474F69A7.9090404>