LVM 1.0.5 patch for Linux 2.4.19-rc3

LVM 1.0.5 patch for Linux 2.4.19-rc3

[Heinz J . Mauelshagen]

All,
have send an LVM 1.0.5 patch to Marcelo directly which addresses:

- an OBO error accessing the vg array
- SMP lock fixes
- using blk_ioctl()
- indenting


It is available at:
<http://people.sistina.com/~mauelshagen/lvm_patches/lvm_1.0.5+_25.07.2002.patch>


Regards,
Heinz    -- The LVM Guy --

=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-

Heinz Mauelshagen                                 Sistina Software Inc.
Senior Consultant/Developer                       Am Sonnenhang 11
                                                  56242 Marienrachdorf
                                                  Germany
Mauelshagen@Sistina.com                           +49 2626 141200
                                                       FAX 924446
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-

Re: LVM 1.0.5 patch for Linux 2.4.19-rc3

[Christoph Hellwig]

On Thu, Jul 25, 2002 at 03:39:44PM +0200, Heinz J . Mauelshagen wrote:
> 
> All,
> have send an LVM 1.0.5 patch to Marcelo directly which addresses:
> 
> - an OBO error accessing the vg array
> - SMP lock fixes
> - using blk_ioctl()
> - indenting

Any estimate when Stephen's non-broken pvmove implementaion will be merged?

Re: LVM 1.0.5 patch for Linux 2.4.19-rc3

[Heinz J . Mauelshagen]

On Thu, Jul 25, 2002 at 03:34:19PM +0100, Christoph Hellwig wrote:
> On Thu, Jul 25, 2002 at 03:39:44PM +0200, Heinz J . Mauelshagen wrote:
> > 
> > All,
> > have send an LVM 1.0.5 patch to Marcelo directly which addresses:
> > 
> > - an OBO error accessing the vg array
> > - SMP lock fixes
> > - using blk_ioctl()
> > - indenting
> 
> Any estimate when Stephen's non-broken pvmove implementaion will be merged?

Probably next month if I get time for that.

-- 

Regards,
Heinz    -- The LVM Guy --

*** Software bugs are stupid.
    Nevertheless it needs not so stupid people to solve them ***

=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-

Heinz Mauelshagen                                 Sistina Software Inc.
Senior Consultant/Developer                       Am Sonnenhang 11
                                                  56242 Marienrachdorf
                                                  Germany
Mauelshagen@Sistina.com                           +49 2626 141200
                                                       FAX 924446
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-

Re: LVM 1.0.5 patch for Linux 2.4.19-rc3

[Christoph Hellwig]

Okay, more comments on the actual patch:


+	/* remove pv's */
+	for(i = 0; i < vg_ptr->pv_max; i++)
+		if(vg_ptr->pv[i]) lvm_fs_remove_pv(vg_ptr, vg_ptr->pv[i]);
+

The code was carefully indented to match Documentation/CodingStyle.
Please don't mix random indentation changes with bugfixes..

--- linux-2.4.19-rc3.orig/drivers/md/lvm-internal.h	Thu Jul 25 13:05:13 2002
+++ linux-2.4.19-rc3/drivers/md/lvm-internal.h	Thu Jul 25 13:46:01 2002
@@ -49,6 +49,10 @@
 extern vg_t *vg[];
 extern struct file_operations lvm_chr_fops;
 
+#ifndef	uchar
+typedef	unsigned char	uchar;
+#endif

Do you _really_ have to use this non-standard type?  can't you use the
BSD u_char or sysv unchar?  and typedef/#define don't really mix nicely..

+#ifdef	list_move
+				list_move(next, hash_table);
+#else
 				list_del(next);
-				list_add(next, hash_table);
+#endif				list_add(next, hash_table);

In 2.5 list_move is a inline function, in 2.4 it is not present at all (yet).
An as LVM is utterly broken on 2.5 anyway this change has _no_ use at all.

 /* variables */
-char *lvm_version =
-    "LVM version " LVM_RELEASE_NAME "(" LVM_RELEASE_DATE ")";
+char *lvm_version = "LVM version "LVM_RELEASE_NAME"("LVM_RELEASE_DATE")";

when you change this anyway, what about const char[] to squeeze out a few bytes?

 struct file_operations lvm_chr_fops = {
-	open:lvm_chr_open,
-	release:lvm_chr_close,
-	ioctl:lvm_chr_ioctl,
+	owner:		THIS_MODULE,
+	open:		lvm_chr_open,
+	release:	lvm_chr_close,
+	ioctl:		lvm_chr_ioctl,
 };

when you update this you could move to C99 initializers, can't you?
 
+static struct gendisk lvm_gendisk =
+{
+	major:		MAJOR_NR,
+	major_name:	LVM_NAME,
+	minor_shift:	0,

this is in .bss, you don't need to initialize to zero.

-}				/* lvm_init() */
+} /* lvm_init() */


can't you just kill those silly end-of-function comments entirely?

 	case 0:
+		down_write(&lv->lv_lock);
 		lv->lv_snapshot_use_rate = lv_rate_req.rate;
+		up_write(&lv->lv_lock);
+		down_read(&lv->lv_lock);

you are sure youreally want to drop the lock here and not downgrade it?
(yes, I'm prodding for the downgrade patch to finally get merged..)

All in all this patch would be _soooo_ much easier to review if you wouldn't
mix random indentation changes with real fixes.

Re: LVM 1.0.5 patch for Linux 2.4.19-rc3

[Marcin Dalecki]

Christoph Hellwig wrote:
>  
> +#ifndef	uchar
> +typedef	unsigned char	uchar;
> +#endif
> 
> Do you _really_ have to use this non-standard type?  can't you use the
> BSD u_char or sysv unchar?  and typedef/#define don't really mix nicely..

Or of course the normal u8 u16 and u32 and infally u64, which are so
much more explicit about the fact that we are actually dealig with
bit slices.

> 
> All in all this patch would be _soooo_ much easier to review if you wouldn't
> mix random indentation changes with real fixes.

Christoph applying the patch and rediffing with diffs "ingore white 
space' options can help you here.
And plese note that this kind of problems wouldn't be that common
if we finally decided to make indent -kr -i8 mandatory.

Re: LVM 1.0.5 patch for Linux 2.4.19-rc3

[Christoph Hellwig]

On Fri, Jul 26, 2002 at 02:47:56AM +0200, Marcin Dalecki wrote:
> Or of course the normal u8 u16 and u32 and infally u64, which are so
> much more explicit about the fact that we are actually dealig with
> bit slices.

*nod*

> > All in all this patch would be _soooo_ much easier to review if you wouldn't
> > mix random indentation changes with real fixes.
> 
> Christoph applying the patch and rediffing with diffs "ingore white 
> space' options can help you here.

It's not that easy - he randomly moves the conditional statements on the
if or else lines and changes brace placement..

> And plese note that this kind of problems wouldn't be that common
> if we finally decided to make indent -kr -i8 mandatory.

nightly run on the bk repo...  Larry, is that possible? :)

<advertise>
for all those who want an indent with sane defaults:

version 0.2 of the 'portable' version of NetBSD is now available.
get it from https://developer.berlios.de/project/filelist.php?group_id=192
and package it for $DISTRO.

feedback welcome.
</advertise>

Re: LVM 1.0.5 patch for Linux 2.4.19-rc3

[Alan Cox]

On Fri, 2002-07-26 at 01:47, Marcin Dalecki wrote:
> Christoph applying the patch and rediffing with diffs "ingore white 
> space' options can help you here.
> And plese note that this kind of problems wouldn't be that common
> if we finally decided to make indent -kr -i8 mandatory.

indent -kr -i8 -bri0  (-l255 is also a good idea) doesn't produce the
same output for all whitespace/wrapped inputs. A long time ago I tried
to work that way and it caused much pain.

Re: LVM 1.0.5 patch for Linux 2.4.19-rc3

[Heinz J . Mauelshagen]

On Fri, Jul 26, 2002 at 02:47:56AM +0200, Marcin Dalecki wrote:
> Christoph Hellwig wrote:
> >  
> > +#ifndef	uchar
> > +typedef	unsigned char	uchar;
> > +#endif
> > 
> > Do you _really_ have to use this non-standard type?  can't you use the
> > BSD u_char or sysv unchar?  and typedef/#define don't really mix nicely..
> 
> Or of course the normal u8 u16 and u32 and infally u64, which are so
> much more explicit about the fact that we are actually dealig with
> bit slices.
> 
> > 
> > All in all this patch would be _soooo_ much easier to review if you wouldn't
> > mix random indentation changes with real fixes.
> 
> Christoph applying the patch and rediffing with diffs "ingore white 
> space' options can help you here.
> And plese note that this kind of problems wouldn't be that common
> if we finally decided to make indent -kr -i8 mandatory.

It should have been for this patch.
Obviously an error on my end.

We'll resend...

-- 

Regards,
Heinz    -- The LVM Guy --

*** Software bugs are stupid.
    Nevertheless it needs not so stupid people to solve them ***

=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-

Heinz Mauelshagen                                 Sistina Software Inc.
Senior Consultant/Developer                       Am Sonnenhang 11
                                                  56242 Marienrachdorf
                                                  Germany
Mauelshagen@Sistina.com                           +49 2626 141200
                                                       FAX 924446
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-