Re: [Drbd-dev] [PATCH 23/27] drbd: make intelligent use of blkdev_issue_zeroout

2018-01-16 Thread Lars Ellenberg 
On Mon, Jan 15, 2018 at 10:07:38AM -0500, Mike Snitzer wrote:
> > See also:
> > https://www.redhat.com/archives/dm-devel/2017-March/msg00213.html
> > https://www.redhat.com/archives/dm-devel/2017-March/msg00226.html
> 
> Right, now that you mention it it is starting to ring a bell (especially
> after I read your 2nd dm-devel archive url above).

> > In tree, either dm-thin learns to do REQ_OP_WRITE_ZEROES "properly",
> > so the result in this scenario is what we expect:
> > 
> >   _: unprovisioned, not allocated, returns zero on read anyways
> >   *: provisioned, some arbitrary data
> >   0: explicitly zeroed:
> > 
> >   |gran|ular|ity ||||
> >   |||||
> >  to|-be-|zero|ed
> >   |**00|||00**|
> > 
> > (leave unallocated blocks alone,
> >  de-allocate full blocks just like with discard,
> >  explicitly zero unaligned head and tail)
> 
> "de-allocate full blocks just like with discard" is an interesting take
> what it means for dm-thin to handle REQ_OP_WRITE_ZEROES "properly".
> 
> > Or DRBD will have to resurrect that reinvented zeroout again,
> > with exactly those semantics. I did reinvent it for a reason ;)
> 
> Yeah, I now recall dropping that line of development because it
> became "hard" (or at least harder than originally thought).
> 
> Don't people use REQ_OP_WRITE_ZEROES to initialize a portion of the
> disk?  E.g. zeroing superblocks, metadata areas, or whatever?
> 
> If we just discarded the logical extent and then a user did a partial
> write to the block, areas that a user might expect to be zeroed wouldn't
> be (at least in the case of dm-thinp if "skip_block_zeroing" is
> enabled).


Oh-kay.
So "immediately after" such an operation
("zero-out head and tail and de-alloc full blocks")
a read to that area would return all zeros, as expected.

But once you do a partial write of something to one of those
de-allocated blocks (and skip_block_zeroing is enabled,
which it likely is due to "performance"),
"magically" arbitrary old garbage data springs into existence
on the LBAs that just before read as zeros.

lvmthin lvm.conf
Would that not break a lot of other things
(any read-modify-write of "upper layers")?
Would that not even be a serious "information leak"
(old garbage of other completely unrelated LVs leaking into this one)?

But thank you for that, I start to see the problem ;-)

> No, dm-thinp doesn't have an easy way to mark an allocated block as
> containing zeroes (without actually zeroing).  I toyed with adding that
> but then realized that even if we had it it'd still require block
> zeroing be enabled.  But block zeroing is done at allocation time.  So
> we'd need to interpret the "this block is zeroes" flag to mean "on first
> write or read to this block it needs to first zero it".  Fugly to say
> the least...


Maybe have a "known zeroed block" pool, allocate only from there,
and "lazy zero" unallocated blocks, add to the known-zero pool?
Fallback to zero-on-alloc if that known-zero-pool is depleted.

Easier said than done, I know.

> But sadly, in general, this is a low priority for me, so you might do
> well to reintroduce your drbd workaround.. sorry about that :(

No problem.
I'll put that back in, and document that we strongly recommend to
NOT skip_block_zeroing in those setups.

Thanks,

Lars

Re: [Drbd-dev] [PATCH 23/27] drbd: make intelligent use of blkdev_issue_zeroout

2018-01-15 Thread Lars Ellenberg 
On Sat, Jan 13, 2018 at 12:46:40AM +, Eric Wheeler wrote:
> Hello All,
> 
> We just noticed that discards to DRBD devices backed by dm-thin devices 
> are fully allocating the thin blocks.
> 
> This behavior does not exist before 
> ee472d83 block: add a flags argument to (__)blkdev_issue_zeroout
> 
> The problem exists somewhere between
> [working] c20cfc27 block: stop using blkdev_issue_write_same for zeroing
>   and
> [broken]  45c21793 drbd: implement REQ_OP_WRITE_ZEROES
> 
> Note that c20cfc27 works as expected, but 45c21793 discards blocks 
> being zeroed on the dm-thin backing device. All commits between those two 
> produce the following error:
> 
> blkdiscard: /dev/drbd/by-res/test: BLKDISCARD ioctl failed: Input/output error
> 
> Also note that issuing a blkdiscard to the backing device directly 
> discards as you would expect. This is just a problem when sending discards 
> through DRBD.
> 
> Is there an easy way to solve this in the short term, even if the ultimate 
> fix is more involved?

> On Wed, 5 Apr 2017, Christoph Hellwig wrote:
> 

commit 0dbed96a3cc9786bc4814dab98a7218753bde934
Author: Christoph Hellwig 
Date:   Wed Apr 5 19:21:21 2017 +0200

drbd: make intelligent use of blkdev_issue_zeroout

> > drbd always wants its discard wire operations to zero the blocks, so
> > use blkdev_issue_zeroout with the BLKDEV_ZERO_UNMAP flag instead of
> > reinventing it poorly.

> > -/*
> > - * We *may* ignore the discard-zeroes-data setting, if so configured.
> > - *
> > - * Assumption is that it "discard_zeroes_data=0" is only because the 
> > backend
> > - * may ignore partial unaligned discards.
> > - *
> > - * LVM/DM thin as of at least
> > - *   LVM version: 2.02.115(2)-RHEL7 (2015-01-28)
> > - *   Library version: 1.02.93-RHEL7 (2015-01-28)
> > - *   Driver version:  4.29.0
> > - * still behaves this way.
> > - *
> > - * For unaligned (wrt. alignment and granularity) or too small discards,
> > - * we zero-out the initial (and/or) trailing unaligned partial chunks,
> > - * but discard all the aligned full chunks.
> > - *
> > - * At least for LVM/DM thin, the result is effectively 
> > "discard_zeroes_data=1".
> > - */
> > -int drbd_issue_discard_or_zero_out(struct drbd_device *device, sector_t 
> > start, unsigned int nr_sectors, bool discard)


As I understood it,
blkdev_issue_zeroout() was supposed to "always try to unmap",
deprovision, the relevant region, and zero-out any unaligned
head or tail, just like my work around above was doing.

And that device mapper thin was "about to" learn this, "soon",
or maybe block core would do the equivalent of my workaround
described above.

But it then did not.

See also:
https://www.redhat.com/archives/dm-devel/2017-March/msg00213.html
https://www.redhat.com/archives/dm-devel/2017-March/msg00226.html

I then did not follow this closely enough anymore,
and I missed that with recent enough kernel,
discard on DRBD on dm-thin would fully allocate.

In our out-of-tree module, we had to keep the older code for
compat reasons, anyways. I will just re-enable our zeroout
workaround there again.

In tree, either dm-thin learns to do REQ_OP_WRITE_ZEROES "properly",
so the result in this scenario is what we expect:

  _: unprovisioned, not allocated, returns zero on read anyways
  *: provisioned, some arbitrary data
  0: explicitly zeroed:

  |gran|ular|ity ||||
  |||||
 to|-be-|zero|ed
  |**00|||00**|

(leave unallocated blocks alone,
 de-allocate full blocks just like with discard,
 explicitly zero unaligned head and tail)

Or DRBD will have to resurrect that reinvented zeroout again,
with exactly those semantics. I did reinvent it for a reason ;)

-- 
: Lars Ellenberg
: LINBIT | Keeping the Digital World Running
: DRBD -- Heartbeat -- Corosync -- Pacemaker
: R, Integration, Ops, Consulting, Support

DRBD® and LINBIT® are registered trademarks of LINBIT

Re: [Drbd-dev] [PATCH 23/27] drbd: make intelligent use of blkdev_issue_zeroout

2018-01-12 Thread Eric Wheeler 
Hello All,

We just noticed that discards to DRBD devices backed by dm-thin devices 
are fully allocating the thin blocks.

This behavior does not exist before 
ee472d83 block: add a flags argument to (__)blkdev_issue_zeroout

The problem exists somewhere between
[working] c20cfc27 block: stop using blkdev_issue_write_same for zeroing
  and
[broken]  45c21793 drbd: implement REQ_OP_WRITE_ZEROES

Note that c20cfc27 works as expected, but 45c21793 discards blocks 
being zeroed on the dm-thin backing device. All commits between those two 
produce the following error:

blkdiscard: /dev/drbd/by-res/test: BLKDISCARD ioctl failed: Input/output error

Also note that issuing a blkdiscard to the backing device directly 
discards as you would expect. This is just a problem when sending discards 
through DRBD.

Is there an easy way to solve this in the short term, even if the ultimate 
fix is more involved?

Thank you for your help!

-Eric

--
Eric Wheeler

On Wed, 5 Apr 2017, Christoph Hellwig wrote:

> drbd always wants its discard wire operations to zero the blocks, so
> use blkdev_issue_zeroout with the BLKDEV_ZERO_UNMAP flag instead of
> reinventing it poorly.
> 
> Signed-off-by: Christoph Hellwig 
> Reviewed-by: Hannes Reinecke 
> ---
>  drivers/block/drbd/drbd_debugfs.c  |   3 --
>  drivers/block/drbd/drbd_int.h  |   6 ---
>  drivers/block/drbd/drbd_receiver.c | 102 
> ++---
>  drivers/block/drbd/drbd_req.c  |   6 +--
>  4 files changed, 7 insertions(+), 110 deletions(-)
> 
> diff --git a/drivers/block/drbd/drbd_debugfs.c 
> b/drivers/block/drbd/drbd_debugfs.c
> index de5c3ee8a790..494837e59f23 100644
> --- a/drivers/block/drbd/drbd_debugfs.c
> +++ b/drivers/block/drbd/drbd_debugfs.c
> @@ -236,9 +236,6 @@ static void seq_print_peer_request_flags(struct seq_file 
> *m, struct drbd_peer_re
>   seq_print_rq_state_bit(m, f & EE_CALL_AL_COMPLETE_IO, , "in-AL");
>   seq_print_rq_state_bit(m, f & EE_SEND_WRITE_ACK, , "C");
>   seq_print_rq_state_bit(m, f & EE_MAY_SET_IN_SYNC, , "set-in-sync");
> -
> - if (f & EE_IS_TRIM)
> - __seq_print_rq_state_bit(m, f & EE_IS_TRIM_USE_ZEROOUT, , 
> "zero-out", "trim");
>   seq_print_rq_state_bit(m, f & EE_WRITE_SAME, , "write-same");
>   seq_putc(m, '\n');
>  }
> diff --git a/drivers/block/drbd/drbd_int.h b/drivers/block/drbd/drbd_int.h
> index 724d1c50fc52..d5da45bb03a6 100644
> --- a/drivers/block/drbd/drbd_int.h
> +++ b/drivers/block/drbd/drbd_int.h
> @@ -437,9 +437,6 @@ enum {
>  
>   /* is this a TRIM aka REQ_DISCARD? */
>   __EE_IS_TRIM,
> - /* our lower level cannot handle trim,
> -  * and we want to fall back to zeroout instead */
> - __EE_IS_TRIM_USE_ZEROOUT,
>  
>   /* In case a barrier failed,
>* we need to resubmit without the barrier flag. */
> @@ -482,7 +479,6 @@ enum {
>  #define EE_CALL_AL_COMPLETE_IO (1<<__EE_CALL_AL_COMPLETE_IO)
>  #define EE_MAY_SET_IN_SYNC (1<<__EE_MAY_SET_IN_SYNC)
>  #define EE_IS_TRIM (1<<__EE_IS_TRIM)
> -#define EE_IS_TRIM_USE_ZEROOUT (1<<__EE_IS_TRIM_USE_ZEROOUT)
>  #define EE_RESUBMITTED (1<<__EE_RESUBMITTED)
>  #define EE_WAS_ERROR   (1<<__EE_WAS_ERROR)
>  #define EE_HAS_DIGEST  (1<<__EE_HAS_DIGEST)
> @@ -1561,8 +1557,6 @@ extern void start_resync_timer_fn(unsigned long data);
>  extern void drbd_endio_write_sec_final(struct drbd_peer_request *peer_req);
>  
>  /* drbd_receiver.c */
> -extern int drbd_issue_discard_or_zero_out(struct drbd_device *device,
> - sector_t start, unsigned int nr_sectors, bool discard);
>  extern int drbd_receiver(struct drbd_thread *thi);
>  extern int drbd_ack_receiver(struct drbd_thread *thi);
>  extern void drbd_send_ping_wf(struct work_struct *ws);
> diff --git a/drivers/block/drbd/drbd_receiver.c 
> b/drivers/block/drbd/drbd_receiver.c
> index dc9a6dcd431c..bc1d296581f9 100644
> --- a/drivers/block/drbd/drbd_receiver.c
> +++ b/drivers/block/drbd/drbd_receiver.c
> @@ -1448,108 +1448,14 @@ void drbd_bump_write_ordering(struct drbd_resource 
> *resource, struct drbd_backin
>   drbd_info(resource, "Method to ensure write ordering: %s\n", 
> write_ordering_str[resource->write_ordering]);
>  }
>  
> -/*
> - * We *may* ignore the discard-zeroes-data setting, if so configured.
> - *
> - * Assumption is that it "discard_zeroes_data=0" is only because the backend
> - * may ignore partial unaligned discards.
> - *
> - * LVM/DM thin as of at least
> - *   LVM version: 2.02.115(2)-RHEL7 (2015-01-28)
> - *   Library version: 1.02.93-RHEL7 (2015-01-28)
> - *   Driver version:  4.29.0
> - * still behaves this way.
> - *
> - * For unaligned (wrt. alignment and granularity) or too small discards,
> - * we zero-out the initial (and/or) trailing unaligned partial chunks,
> - * but discard all the aligned full chunks.
> - *
> - * At least for LVM/DM thin, the result is effectively 
> "discard_zeroes_data=1".
> - */
> -int