[Drbd-dev] invalidate broken in 8.3.0 regression from 8.2.6

[Drbd-dev] invalidate broken in 8.3.0 regression from 8.2.6

[ILLES, Marton]

Hi,

First of all thanks for drbd, it is such a nice piece of code, we love
to use it.

In my current setup I use drbd 8.3.0 with linux 2.6.27 and some ubuntu
patches (but it does not really matter), while I discovered that
"drbdsetup /dev/drbd0 invalidate" does not do anything. No error code,
nothing. In 8.2.6 it worked well, so some regression is here.

After some debugging and git digging I realized that invalidate logic
was changed in this patch:

commit 1ad8484c83eb1ae28a8471d998c7b060ed045493
Author: Philipp Reisner <philipp.reisner@linbit.com>
Date:   Fri May 9 10:33:35 2008 +0200

    Mande the invalidate and invalidate-remote to fit with the
"before-resync-target" handler
    
    Changed the code so that with the invalidate and invalidate-remote
    commands the disk state is only set to invalidate _after_ the
    before-resync-target handler returned success.


However it is way before 8.2.6, so it should not cause problem. After
some more careful code watching, I realized that after 8.2.6 release
during a tree merge the fix it is partially reverted, cause currently we
have:

 861     /* Early state sanitising. */
 862 
 863     /* Dissalow the invalidate command to connect  */
 864     if ((ns.conn == StartingSyncS || ns.conn == StartingSyncT) &&
 865         os.conn < Connected) {
 866         ns.conn = os.conn;
 867         ns.pdsk = os.pdsk;
 868     }

While the 1ad8484c83eb1ae28a8471d998c7b060ed045493 patch was:
--- a/drbd/drbd_main.c
+++ b/drbd/drbd_main.c
@@ -707,6 +707,10 @@ int is_valid_state_transition(struct drbd_conf *mdev,
            ns.conn != os.conn && os.conn > Connected)
                rv = SS_ResyncRunning;
 
+       if ((ns.conn == StartingSyncS || ns.conn == StartingSyncT) &&
+           os.conn < Connected)
+               rv = SS_NeedConnection;
+
        return rv;
 }
 
@@ -730,12 +734,7 @@ int _drbd_set_state(struct drbd_conf *mdev,
                dec_local(mdev);
        }
 
-       /* Early state sanitising. Dissalow the invalidate ioctl to connect  */
-       if ( (ns.conn == StartingSyncS || ns.conn == StartingSyncT) &&
-               os.conn < Connected ) {
-               ns.conn = os.conn;
-               ns.pdsk = os.pdsk;
-       }
+       /* Early state sanitising. */
 
        /* Dissalow Network errors to configure a device's network part */
        if ( (ns.conn >= Timeout && ns.conn <= TearDown ) &&

So IMHO some lines come back accidentally, which is bad. Checking the
patch it looks like that is the only affected part that "returned".
After removing the lines, invalidate works like a charm.

So here is the "magic" patch to fix it:

diff --git a/drbd/drbd_main.c b/drbd/drbd_main.c
index 1a66e52..d73e247 100644
--- a/drbd/drbd_main.c
+++ b/drbd/drbd_main.c
@@ -860,13 +860,6 @@ int _drbd_set_state(struct drbd_conf *mdev,
 
        /* Early state sanitising. */
 
-       /* Dissalow the invalidate command to connect  */
-       if ((ns.conn == StartingSyncS || ns.conn == StartingSyncT) &&
-               os.conn < Connected) {
-               ns.conn = os.conn;
-               ns.pdsk = os.pdsk;
-       }
-
        /* Dissalow Network errors to configure a device's network part */
        if ((ns.conn >= Timeout && ns.conn <= TearDown) &&
            os.conn <= Disconnecting)

Please correct me if I am wrong, or please apply the patch. :)

Maybe it would make sense to check how it could happen and check other
parts of the code as well...

thanks

Marton

PS: I am off list, so please CC-me.
-- 
Key fingerprint = F78C 25CA 5F88 6FAF EA21 779D 3279 9F9E 1155 670D

Re: [Drbd-dev] invalidate broken in 8.3.0 regression from 8.2.6

[Lars Ellenberg]

On Wed, Mar 11, 2009 at 06:30:21PM +0100, ILLES, Marton wrote:
> Hi,
> 
> First of all thanks for drbd, it is such a nice piece of code, we love
> to use it.
> 
> In my current setup I use drbd 8.3.0 with linux 2.6.27 and some ubuntu
> patches (but it does not really matter), while I discovered that
> "drbdsetup /dev/drbd0 invalidate" does not do anything. No error code,
> nothing. In 8.2.6 it worked well, so some regression is here.

was drbd0 configured while you did "drbdsetup /dev/drbd0 invalidate"?

if not, that is a known bug, which we unfortunately not have fixed yet.
drbdadm should try drbdsetup, and if that fails because the device is
unconfigured, it should use drbdmeta to mark it as invalid in the meta
data itself.

unfortunately drbdsetup invalidate exits with 0 for unconfigured
devices.

if drbdsetup invalidate does nothing even while it is configured,
that would be new to me.

> After
> some more careful code watching, I realized that after 8.2.6 release
> during a tree merge the fix it is partially reverted, cause currently we
> have:
> 
>  861     /* Early state sanitising. */
>  862 
>  863     /* Dissalow the invalidate command to connect  */
>  864     if ((ns.conn == StartingSyncS || ns.conn == StartingSyncT) &&
>  865         os.conn < Connected) {
>  866         ns.conn = os.conn;
>  867         ns.pdsk = os.pdsk;
>  868     }
> 
> While the 1ad8484c83eb1ae28a8471d998c7b060ed045493 patch was:
> --- a/drbd/drbd_main.c
> +++ b/drbd/drbd_main.c
> @@ -707,6 +707,10 @@ int is_valid_state_transition(struct drbd_conf *mdev,
>             ns.conn != os.conn && os.conn > Connected)
>                 rv = SS_ResyncRunning;
>  
> +       if ((ns.conn == StartingSyncS || ns.conn == StartingSyncT) &&
> +           os.conn < Connected)
> +               rv = SS_NeedConnection;
> +
>         return rv;
>  }
>  
> @@ -730,12 +734,7 @@ int _drbd_set_state(struct drbd_conf *mdev,
>                 dec_local(mdev);
>         }
>  
> -       /* Early state sanitising. Dissalow the invalidate ioctl to connect  */
> -       if ( (ns.conn == StartingSyncS || ns.conn == StartingSyncT) &&
> -               os.conn < Connected ) {
> -               ns.conn = os.conn;
> -               ns.pdsk = os.pdsk;
> -       }
> +       /* Early state sanitising. */
>  
>         /* Dissalow Network errors to configure a device's network part */
>         if ( (ns.conn >= Timeout && ns.conn <= TearDown ) &&
> 
> So IMHO some lines come back accidentally, which is bad. Checking the
> patch it looks like that is the only affected part that "returned".
> After removing the lines, invalidate works like a charm.
> 
> So here is the "magic" patch to fix it:
> 
> diff --git a/drbd/drbd_main.c b/drbd/drbd_main.c
> index 1a66e52..d73e247 100644
> --- a/drbd/drbd_main.c
> +++ b/drbd/drbd_main.c
> @@ -860,13 +860,6 @@ int _drbd_set_state(struct drbd_conf *mdev,
>  
>         /* Early state sanitising. */
>  
> -       /* Dissalow the invalidate command to connect  */
> -       if ((ns.conn == StartingSyncS || ns.conn == StartingSyncT) &&
> -               os.conn < Connected) {
> -               ns.conn = os.conn;
> -               ns.pdsk = os.pdsk;
> -       }
> -
>         /* Dissalow Network errors to configure a device's network part */
>         if ((ns.conn >= Timeout && ns.conn <= TearDown) &&
>             os.conn <= Disconnecting)
> 
> Please correct me if I am wrong, or please apply the patch. :)

thanks for reporting and investigating,
we'll have a look into why code changed as it did,
and what in fact is wrong.


-- 
: Lars Ellenberg
: LINBIT | Your Way to High Availability
: DRBD/HA support and consulting http://www.linbit.com

DRBD® and LINBIT® are registered trademarks of LINBIT, Austria.

Re: [Drbd-dev] invalidate broken in 8.3.0 regression from 8.2.6

[Lars Ellenberg]

On Wed, Mar 11, 2009 at 07:58:32PM +0100, ILLES, Marton wrote:
> On Wed, 2009-03-11 at 19:35 +0100, Lars Ellenberg wrote:
> > On Wed, Mar 11, 2009 at 06:30:21PM +0100, ILLES, Marton wrote:
> > > Hi,
> > > 
> > > First of all thanks for drbd, it is such a nice piece of code, we love
> > > to use it.
> > > 
> > > In my current setup I use drbd 8.3.0 with linux 2.6.27 and some ubuntu
> > > patches (but it does not really matter), while I discovered that
> > > "drbdsetup /dev/drbd0 invalidate" does not do anything. No error code,
> > > nothing. In 8.2.6 it worked well, so some regression is here.
> > 
> > was drbd0 configured while you did "drbdsetup /dev/drbd0 invalidate"?
> > 
> > if not, that is a known bug, which we unfortunately not have fixed yet.
> > drbdadm should try drbdsetup, and if that fails because the device is
> > unconfigured, it should use drbdmeta to mark it as invalid in the meta
> > data itself.
> > 
> > unfortunately drbdsetup invalidate exits with 0 for unconfigured
> > devices.
> > 
> > if drbdsetup invalidate does nothing even while it is configured,
> > that would be new to me.
> > 
> 
> Hi,
> 
> It was configured in StandAlone Secondary/DUnknown state. I do not use
> drbdadm only drbdsetup. Though I still believe that the cause was the
> missed patch.

ok.

drbdadm invalidate
 - works as intended when connected
	(also for you?)
 - is broken when standalone in drbd 8.3
	(regression as per this report)
 - is broken when unconfigured (possibly for a long time already).
	(I found out a while back)

we'll fix that.

-- 
: Lars Ellenberg
: LINBIT | Your Way to High Availability
: DRBD/HA support and consulting http://www.linbit.com

DRBD® and LINBIT® are registered trademarks of LINBIT, Austria.

Re: [Drbd-dev] invalidate broken in 8.3.0 regression from 8.2.6

[Philipp Reisner]

On Wednesday 11 March 2009 18:30:21 ILLES, Marton wrote:
> Hi,
>
> First of all thanks for drbd, it is such a nice piece of code, we love
> to use it.
>
> In my current setup I use drbd 8.3.0 with linux 2.6.27 and some ubuntu
> patches (but it does not really matter), while I discovered that
> "drbdsetup /dev/drbd0 invalidate" does not do anything. No error code,
> nothing. In 8.2.6 it worked well, so some regression is here.
>
> After some debugging and git digging I realized that invalidate logic
> was changed in this patch:
>

Thanks a lot for pointing this out!

See...
http://git.drbd.org/?p=drbd-8.3.git;a=commitdiff;h=a8403ade52180175a0e657770d8698abe394a707

best,
 Philipp
-- 
: Dipl-Ing Philipp Reisner
: LINBIT | Your Way to High Availability
: Tel: +43-1-8178292-50, Fax: +43-1-8178292-82
: http://www.linbit.com

DRBD(R) and LINBIT(R) are registered trademarks of LINBIT, Austria.