g_journal_ufs_using_last_sector() needs minor correction (patch included)

[Pawel Jakub Dawidek]

On Sat, Jan 05, 2013 at 08:03:04PM +0200, Konstantin Belousov wrote:
> 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 ?

No, patch looks good to me.

-- 
Pawel Jakub Dawidek                       http://www.wheelsystems.com
FreeBSD committer                         http://www.FreeBSD.org
Am I Evil? Yes, I Am!                     http://tupytaj.pl
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-fs/attachments/20130105/be00c97d/attachment.sig>