[Drbd-dev] [PATCH] drbd: fix discard_zeroes_if_aligned regression

[Eric Wheeler]

On Mon, 3 Jun 2019, Lars Ellenberg wrote:
> On Sun, Jun 02, 2019 at 12:28:31AM +0000, Eric Wheeler wrote:
> > On Tue, 28 May 2019, Lars Ellenberg wrote:
> > 
> > > On Fri, May 10, 2019 at 05:36:32PM +0000, Eric Wheeler wrote:
> > > > Hi Lars,
> > > > 
> > > > We just tried 4.19.x and this bugs still exists. We applied the patch 
> > > > which was originally submitted to this thread and it still applies cleanly 
> > > > and seems to work for our use case. You mentioned that you had some older 
> > > > code which zeroed out unaligned discard requests (or perhaps it was for a 
> > > > different purpose) that you may be able to use to patch this. Could you 
> > > > dig those up and see if we can get this solved?
> > > > 
> > > > It would be nice to be able to use drbd with thin backing volumes from the 
> > > > vanilla kernel. If this has already been fixed in something newer than 
> > > > 4.19, then please point me to the commit.
> > > 
> > > I think it was merged upstream in 5.0
> > > f31e583aa2c2 drbd: introduce P_ZEROES (REQ_OP_WRITE_ZEROES on the "wire")
> > 
> > Thanks Lars, I appreciate your patch.  
> > 
> > Your unaligned zerout code in drbd_issue_discard_or_zero_out() looks 
> > great.  I particulary like how you adjusted max_discard_sectors to the 
> > granularity, as well as alignment handling.  Well thought out.
> > 
> > Your commit notes that "for backward compatibility, P_TRIM means zero-out, 
> > unless the DRBD_FF_WZEROES feature flag is agreed upon during handshake."
> > 
> > We test our environment by deploying the newer kernel on one of the DRBD 
> > servers and checking for regressions---but this will cause a zero-out on 
> > the new server because the old server doesn't yet support DRBD_FF_WZEROES.
> > 
> > For our purpose, can you think of any reason that it would be unsafe to 
> > hack the following into drbd_do_features() so the newer version will not 
> > zero-out while we test and get both nodes up to the newer version?
> 
> If you "fake" agreed_features to include DRBD_FF_WZEROES,
> that may start to send the P_ZEROES command,
> which the older peer does not understand yet,
> triggering a protocol error and disconnect...

Good point.

> You can always use a newer DRBD module with the older kernel,
> until you are prepared to upgrade the kernel...
> 
> But you knew that.

True.  I was hoping for a way to test without making any changes to the 
first node, but I'll give it a shot.  At least we can stay on the older 
kernel version and only upgrade the drbd module.

Thanks for the feedback!

--
Eric Wheeler



> 
> > diff --git a/drivers/block/drbd/drbd_receiver.c b/drivers/block/drbd/drbd_receiver.c
> > index c7ad88d..76191e6 100644
> > --- a/drivers/block/drbd/drbd_receiver.c
> > +++ b/drivers/block/drbd/drbd_receiver.c
> > @@ -5382,6 +5382,8 @@ static int drbd_do_features(struct drbd_connection *connection)
> >  	connection->agreed_pro_version = min_t(int, PRO_VERSION_MAX, p->protocol_max);
> >  	connection->agreed_features = PRO_FEATURES & be32_to_cpu(p->feature_flags);
> >  
> > +	connection->agreed_features |= DRBD_FF_WZEROES;
> > +
> >  	drbd_info(connection, "Handshake successful: "
> >  	     "Agreed network protocol version %d\n", connection->agreed_pro_version);
> 
> -- 
> : Lars Ellenberg
> : LINBIT | Keeping the Digital World Running
> : DRBD -- Heartbeat -- Corosync -- Pacemaker
> : R&D, Integration, Ops, Consulting, Support
> 
> DRBD® and LINBIT® are registered trademarks of LINBIT
> _______________________________________________
> drbd-dev mailing list
> drbd-dev at lists.linbit.com
> http://lists.linbit.com/mailman/listinfo/drbd-dev
>