g_journal_ufs_using_last_sector() needs minor correction (patch included)

[Konstantin Belousov]

On Wed, Jan 02, 2013 at 11:07:24PM +0100, Andreas Longwitz wrote:
> On a testdisk created with
> 
>   dd if=/dev/zero of=diskfile count=5000 bs=1m
>   mdconfig -a -t vnode -f diskfile -u 1
> 
> let us create a mirrored and gjournaled filesystem:
> 
> gmirror label -b prefer -F gmX md1
> gpart create -s GPT mirror/gmX
> gpart add -t freebsd-swap -s 4194304 -i 1 mirror/gmX
> gpart add -t freebsd-ufs -i 2 mirror/gmX
> gjournal label mirror/gmXp2 mirror/gmXp1
> newfs -J /dev/mirror/gmXp2.journal
> mount /dev/mirror/gmXp2.journal /mnt
> ...  put some data to /mnt
> umount /mnt
> 
> The mediasizes in sectors for gjournal consumer and provider:
> 
> diskinfo /dev/mirror/gmXp2.journal -->
> /dev/mirror/gmXp2.journal     512     3095361024      6045627 0    0
> 
> dumpfs -m /dev/mirror/gmXp2.journal -->
> # newfs command for /dev/mirror/gmXp2.journal
> newfs -O 2 -J -a 8 -b 16384 -d 16384 -e 2048 -f 2048 -g 16384 -h 64
> -m 8 -o time -s 6045624 /dev/mirror/gmXp2.journal
> 
> The size of the created filesystem is 6045624 and three blocks less than
> the size gjournal provides.
> 
> Now I had the need to rename gmX to gmY without loosing my data:
> 
> gjournal stop mirror/gmXp2.journal
> gjournal clear mirror/gmXp2
> gjournal clear mirror/gmXp1
> gpart delete -i 1 mirror/gmX
> gpart delete -i 2 mirror/gmX
> gpart destroy mirror/gmX
> gmirror stop gmX
> gmirror clear /dev/md1
> gmirror label -b prefer -F gmY md1
> gpart create -s GPT mirror/gmY
> gpart add -t freebsd-swap -s 4194304 -i 1 mirror/gmY
> gpart add -t freebsd-ufs -i 2 mirror/gmY
> gjournal label mirror/gmYp2 mirror/gmYp1
> 
> At this point gjournal gives the unjustified error message
> "File system on mirror/gmYp2 is using the last
> sector and this operation is going to overwrite
> it. Use -f if you really want to do it."
> 
> the reason for this is an improper size check in the function
> g_journal_ufs_using_last_sector() which can be corrected by the
> following patch:
> 
> --- geom_journal_ufs.c.orig     2009-08-03 10:13:06.000000000 +0200
> +++ geom_journal_ufs.c  2013-01-02 23:01:37.000000000 +0100
> @@ -73,6 +73,6 @@
>         /* Provider size in 512 bytes blocks. */
>         psize = g_get_mediasize(prov) / DEV_BSIZE;
>         /* File system size in 512 bytes blocks. */
> -       fssize = fsbtodb(fs, dbtofsb(fs, psize));
> -       return (psize == fssize);
> +       fssize = fsbtodb(fs, fs->fs_size);
> +       return (psize <= fssize);
>  }

I agree with your analysis, the patch looks right.
Existing code rounded the filesystem size up to the fragment size,
which is typically 8 disk blocks. But, in fact, only the last disk
block matters.

Pawel, do you have any comments ?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 834 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-fs/attachments/20130105/e12ec453/attachment.sig>