Re: Re: fr1-2.16 patch for 2.6.10 kernel

Peter T. Breuer a écrit :
> "Also sprach [email protected]:"
> [Charset ISO-8859-1 unsupported, filtering to ASCII...]
> 
>>Selon "Peter T. Breuer" <[email protected]>:
>>
>>OK, it compiles with just some warnings. I will investigate it later
>>I send you the patch. I have  just forgotten to suppress in my patch these lines
>>:  
>>
>> #ifdef CONFIG_MD_RAID1_ROBUST_READ
>>        mdk_rdev_t *rdev = *rdevp;
>> #endif /* CONFIG_MD_RAID1_READ_WRITE_CORRECT */
> 
> 
> Sure. No problem. I'm worried about the warnings, though! There should
> be none.
> 
> I'm just looking at the patch now. Would be easier with the .orig files
> to hand too.
> 
> In md.c, I think this bit brobably wants changing:
> 
>                 if (currspeed > sysctl_speed_limit_min) {
> +#ifdef MD_BITMAP_SUPPORT
> +                        /* PTB use realspeed for upper limit on i/o */
> +                       if ((realspeed > sysctl_speed_limit_max) ||
> +                                       !is_mddev_idle(mddev)) {
> +#else
>                         if ((currspeed > sysctl_speed_limit_max) ||
>                                         !is_mddev_idle(mddev)) {
> -                               msleep_interruptible(250);
> +#endif /* MD_BITMAP_SUPPORT */
> +                               current->state = TASK_INTERRUPTIBLE;
> +                               schedule_timeout(HZ/4);
>                                 goto repeat;
>                         }
> 
> should probably be instead
> 
>                 if (currspeed > sysctl_speed_limit_min) {
> +#ifdef MD_BITMAP_SUPPORT
> +                        /* PTB use realspeed for upper limit on i/o */
> +                       if ((realspeed > sysctl_speed_limit_max) ||
> +                                       !is_mddev_idle(mddev)) {
> +#else
>                         if ((currspeed > sysctl_speed_limit_max) ||
>                                         !is_mddev_idle(mddev)) {
>                                 msleep_interruptible(250);
> +#endif /* MD_BITMAP_SUPPORT */
>                                 goto repeat;
>                         }
> 

Ok, done.
> 
> (why did you add the other stuff instead of the msleep? Was it there in
> the previous patch? Oh well).
> 

> It's interesting that a bit later down you had to ADD an extra
> 
> +                       wake_up(&resync_wait);
> 
> and it's probably quite right. I would imagine, without seeing the
> original code, that the omission was a newly introduced code bug. But
> check that that wait queue is still defined in the code!
> 
> I've also noticed that the declaration at top of md.c:
> 
> +#ifdef MD_BITMAP_SUPPORT
> +/* PTB md_throttle permits speed calculation adjustments from personality */
> +static atomic_t md_throttle[MAX_MD_DEVS];
> +#endif /* MD_BITMAP_SUPPORT */
> 
> may not be enough in the case when all drivers are compiled in. Or
> perhaps that's only in 2.4, where the later EXPORT_SYMBOL call doesn't
> seem to overcome the static declaration here.  Anyway, I don't think the
> following can be harmful:
> 
> 
> +#ifdef MD_BITMAP_SUPPORT
> +/* PTB md_throttle permits speed calculation adjustments from personality */
> +ifdef MODULE
> +static
> +endif /* MODULE */
> +atomic_t md_throttle[MAX_MD_DEVS];
> +#endif /* MD_BITMAP_SUPPORT */

Done.

> 
> 
> I couldn't even notice any difference in the md.c patch beyond the areas
> I remarked on above.
> 
> As to raid1.c ...
> 
> Hmm ... well, I'm confused! You seem to have added IN a map() function.
> That can't be right! It's meant to be a modification of whatever
> function is already there and is used to redirect retried reads to
> their target. Are you sure it hasn't been renamed to "raid1_remap" or
> something actually meaningful ("map" never was a good name and it
> wouldn't surprise me if it had been changed!)?
> 

For the moment, i have suppressed map() and i  try to use read_balance() 
  instead. Maybe there are still a couple of things to change on it? I 
will investigate it later.

> Anyway, you've still got the " && rdev != *rdevp" in your map function,
> which shouldn't be there, I suppose. Or at least, not until somebody
> thinks about it and decides!
> 
> But for the moment you need to locate the renamed "map" function.
> 
> Mmm .. this bit must be wrong:
> 
> -       rcu_read_unlock();
> +#ifdef CONFIG_MD_FR1
> +        /* zero the bad disk count on the r1bio by default */
> +        r1_bio->nonoperational = 0;
> +#endif /* CONFIG_MD_FR1 */
> +       spin_unlock_irq(&conf->device_lock);
> 
> It must be that the spin unlock has changed to be "rcu_read_unlock".
> You'll have to check what spinlocks are used! But you just can't remove
> an unlock! Look and see what is used and use it. I imagine you want:
> 
> -       rcu_read_unlock();
> +#ifdef CONFIG_MD_FR1
> +        /* zero the bad disk count on the r1bio by default */
> +        r1_bio->nonoperational = 0;
> +#endif /* CONFIG_MD_FR1 */
> +       rcu_read_unlock();
> 
Ok, it is clear here
> 
> Yes - you see, in the original patch, the spin unlock is not added,
> it's there already!  So you can't just add it now.  It will be
> unbalanced.  And that makes me suspect that the spinlocks have changed
> completely, which gives the patch little chance of working as it is.
> The locks will have to be investigated. If you pass me raid1.c I can
> opine a bit more (wheee - kibbitzing is so much more fun!).
> 
> Oooh .. lower down there are some more lock shenanigans which cannot be
> right:
> 
> @@ -714,6 +1053,7 @@
>         conf_t *conf = mddev->private;
>         mirror_info_t *tmp;
> 
> +       spin_lock_irq(&conf->device_lock);
>         /*
>          * Find all failed disks within the RAID1 configuration
>          * and mark them readable
> @@ -728,6 +1068,7 @@
>                         tmp->rdev->in_sync = 1;
>                 }
>         }
> +       rcu_read_unlock();
> 
>         print_conf(conf);
>         return 0;
> @@ -741,8 +1082,23 @@
>         int mirror;
>         mirror_info_t *p;
> 
> +       spin_lock_irq(&conf->device_lock);
>         for (mirror=0; mirror < mddev->raid_disks; mirror++)
> 
> 
> By the look of it there is a spin_lock added and a rcu_unlock balances
> it! It must be one or the other, no? Not a mixture!
> 
> Look and see what is the proper guard for reading the config.
> 

> And that last spin lock seems to be unbalanced! It looks well
> intentioned, but better not to add it at all and take ones chances with
> a race condition than add it and not unlock!
> 
In the original 2.6.10, all these locks are removed.  So I did too.

> Hmmm .. this bit also cannot be right:
> 
> +#else
>                 if ( !(p=conf->mirrors+mirror)->rdev) {
> +                       p->rdev = rdev;
> +#endif /* CONFIG_MD_FR1 */
> 
> since it's in the #else branch, ie. the normal undisturbed code. You
> shouldn't be adding anything in there! So no assignment.
> 
> And the #if case a bit higher up mirrors the else case and cannot
> really be right, therefore ... let's look at it:
> 
> 
> +#ifdef CONFIG_MD_FR1
> +            /*
> +             * allow a disk which has only been set faulty but not
> +             * removed yet to be reinserted, thus triggering a hot
> +             * repair.
> +             */
> +               if ( !(p=conf->mirrors+mirror)->rdev
> +                    || p->rdev == rdev
> +                    || p->rdev->bdev->bd_inode->i_rdev == rdev->bdev->bd_inode->i_rdev) {
> +                        if (!p->rdev)
> +                               p->rdev = rdev;
> +#else
>                 if ( !(p=conf->mirrors+mirror)->rdev) {
> +                       p->rdev = rdev;
> +#endif /* CONFIG_MD_FR1 */
> 
> OK, well, if they don't assign rdeb, I suppose we can't either:
> 
> +#ifdef CONFIG_MD_FR1
> +            /*
> +             * allow a disk which has only been set faulty but not
> +             * removed yet to be reinserted, thus triggering a hot
> +             * repair.
> +             */
> +               if ( !(p=conf->mirrors+mirror)->rdev
> +                    || p->rdev == rdev
> +                    || p->rdev->bdev->bd_inode->i_rdev == rdev->bdev->bd_inode->i_rdev) {
> +#else
>                 if ( !(p=conf->mirrors+mirror)->rdev) {
> +#endif /* CONFIG_MD_FR1 */
> 

Ok, changed it.

> and that bit of code will have to be watched very carefully. I seem to
> recall having to work out what needs to be set to make the array
> believe it has already done a hotadd. That debugging work may heve to be
> repeated. I recall that it was hard work, but perhaps what hotadd does
> is more obvious now.
> 
> In one of the header files there is something interesting:
> 
> -       __u8    pad3[64-32];    /* set to 0 when writing */
change introduced by 2.6.10 ?
> +       __u32 bitmap_events_lo; /* low-order bits of bitmap events count   */
> +       __u32 bitmap_events_hi; /* high-order bits of bitmap events count  */
> +        /* Begin macros to use 2 ints to support fast raid */
> +#define MD_SB_BITMAP_EVENTS_LO_1(sb)     (sb)->bitmap_events_lo
> +#define MD_SB_BITMAP_EVENTS_HI_1(sb)     (sb)->bitmap_events_hi
> +#define MD_SB_BITMAP_EVENTS_1(sb) \
> +    (((u64)MD_SB_BITMAP_EVENTS_HI_1(sb) << 32)|((u64)MD_SB_BITMAP_EVENTS_LO_1(sb)))
> +        /* End macros to use 2 ints to support fast raid */
> +       __u8    pad3[64-40];    /* set to 0 when writing */
> 
> 
> I think you are quite right, but it's interesting that the pad3[64-32]
> was not in the original patch. It looks as though the kernel had a code
> error there originally, and that error has now been corrected. My
> original patch corrected it by filling out the miscount with two extra
> ints so that the miscount became correct, and now your patch maintains
> that correctness, but now it actually has a correct byte count to
> change!
> 
> That probably means that maybe raid disks from 2.6.8.1. are not
> compatible with raid disks from 2.6.10! I haven't looked further.
> 
> 
> Anyway, in summary: spinlocks need study. They're wrong at the moment.
> Couple of other minor things too, but otherwise OK looking!
> 
> 
> Peter
> 
> 
> 
> 
>