Re: [patch 106/327] ufs: Fix mount check in ufs_fill_super()

[Evgeniy Dushistov <dushistov@mail.ru>]

On Tue, Oct 16, 2007 at 11:26:53PM -0700, akpm@linux-foundation.org wrote:
> From: Satyam Sharma <satyam@infradead.org>
> 
> The current code skips the check to verify whether the filesystem was
> previously cleanly unmounted, if (flags & UFS_ST_MASK) == UFS_ST_44BSD or
> UFS_ST_OLD.  This looks like an inadvertent bug that slipped in due to
> parantheses in the compound conditional to me, especially given that
> ufs_get_fs_state() handles the UFS_ST_44BSD case perfectly well.  So, let's
> fix the compound condition appropriately.
> 

I wonder on what type of UFS do you test this patch?
NetBSD and FreeBSD do not use "fs_state", they use "fs_clean" flag,
only Solaris does check like this: fs_state + fs_time == FSOK.

That's why parentheses was like that.

At now with linux-2.6.24-rc1-git1, I get: fs need fsck,
but NetBSD's fsck says that's all ok.

I suggest revert this patch.

> Signed-off-by: Satyam Sharma <satyam@infradead.org>
> Cc: Evgeniy Dushistov <dushistov@mail.ru>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  fs/ufs/super.c |   15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff -puN fs/ufs/super.c~ufs-fix-sun-state-fix-mount-check-in-ufs_fill_super fs/ufs/super.c
> --- a/fs/ufs/super.c~ufs-fix-sun-state-fix-mount-check-in-ufs_fill_super
> +++ a/fs/ufs/super.c
> @@ -933,19 +933,20 @@ magic_found:
>  		goto again;
>  	}
>  
> -	sbi->s_flags = flags;/*after that line some functions use s_flags*/
> +	/* Set sbi->s_flags here, used by ufs_get_fs_state() below */
> +	sbi->s_flags = flags;
>  	ufs_print_super_stuff(sb, usb1, usb2, usb3);
>  
>  	/*
>  	 * Check, if file system was correctly unmounted.
>  	 * If not, make it read only.
>  	 */
> -	if (((flags & UFS_ST_MASK) == UFS_ST_44BSD) ||
> -	  ((flags & UFS_ST_MASK) == UFS_ST_OLD) ||
> -	  (((flags & UFS_ST_MASK) == UFS_ST_SUN || 
> -	    (flags & UFS_ST_MASK) == UFS_ST_SUNOS ||
> -	  (flags & UFS_ST_MASK) == UFS_ST_SUNx86) && 
> -	  (ufs_get_fs_state(sb, usb1, usb3) == (UFS_FSOK - fs32_to_cpu(sb, usb1->fs_time))))) {
> +	if ((((flags & UFS_ST_MASK) == UFS_ST_44BSD)	||
> +	     ((flags & UFS_ST_MASK) == UFS_ST_OLD)	||
> +	     ((flags & UFS_ST_MASK) == UFS_ST_SUN)	||
> +	     ((flags & UFS_ST_MASK) == UFS_ST_SUNOS)	||
> +	     ((flags & UFS_ST_MASK) == UFS_ST_SUNx86))	&&
> +	    (ufs_get_fs_state(sb, usb1, usb3) == (UFS_FSOK - fs32_to_cpu(sb, usb1->fs_time)))) {
>  		switch(usb1->fs_clean) {
>  		case UFS_FSCLEAN:
>  			UFSD("fs is clean\n");
> _

-- 
/Evgeniy