Re: [PATCH 0/2] Fix blkdev_issue_discard()

Re: [PATCH 0/2] Fix blkdev_issue_discard()

[Mike Snitzer]

On Tue, Dec 15 2015 at  9:38pm -0500,
Martin K. Petersen <martin.petersen@oracle.com> wrote:

> >>>>> "Bart" == Bart Van Assche <bart.vanassche@sandisk.com> writes:
> 
> Bart> Should the caller of blkdev_issue_discard() implement this or
> Bart> should this functionality be added in blkdev_issue_discard()
> Bart> itself ?
> 
> I'm very much against dropping information in the ioctl/filesystem
> submission path. But I am not against having a helper function of some
> sort that DM and target could use to handle heads and tails.
> 
> Right now DM does not handle this and I think it would be worthwhile to
> have. Mike, what are your requirements?

If the discard doesn't conform to the device's stacked discard limits
then yes the head and/or tail will get dropped on the floor.

Not sure why DM is being made the focal point on this.  Are you thinking
specifically about DM-thinp and its discard_granularity?  DM thinp
doesn't support partial thinp block discards -- so misaligned/partial
discards are dropped by DM thinp.  There isn't a compelling case for
fixing this (by adding a bitset for each thinp block to support finer
grained discards) -- at least not that I'm aware of.

But I'm missing exactly what it is your helper function would do... and
yet you're asking me what my requirements are.. sorry ;)

Re: [PATCH 0/2] Fix blkdev_issue_discard()

[Jan Kara]

On Wed 16-12-15 00:12:23, Mike Snitzer wrote:
> On Tue, Dec 15 2015 at  9:38pm -0500,
> Martin K. Petersen <martin.petersen@oracle.com> wrote:
> 
> > >>>>> "Bart" == Bart Van Assche <bart.vanassche@sandisk.com> writes:
> > 
> > Bart> Should the caller of blkdev_issue_discard() implement this or
> > Bart> should this functionality be added in blkdev_issue_discard()
> > Bart> itself ?
> > 
> > I'm very much against dropping information in the ioctl/filesystem
> > submission path. But I am not against having a helper function of some
> > sort that DM and target could use to handle heads and tails.
> > 
> > Right now DM does not handle this and I think it would be worthwhile to
> > have. Mike, what are your requirements?
> 
> If the discard doesn't conform to the device's stacked discard limits
> then yes the head and/or tail will get dropped on the floor.
> 
> Not sure why DM is being made the focal point on this.  Are you thinking
> specifically about DM-thinp and its discard_granularity?  DM thinp
> doesn't support partial thinp block discards -- so misaligned/partial
> discards are dropped by DM thinp.  There isn't a compelling case for
> fixing this (by adding a bitset for each thinp block to support finer
> grained discards) -- at least not that I'm aware of.
> 
> But I'm missing exactly what it is your helper function would do... and
> yet you're asking me what my requirements are.. sorry ;)

I agree that just dropping head & tail from misaligned discard request
looks reasonable. However I think the original motivation for this thread
was actually blkdev_issue_zeroout(). That ends up being implemented using
blkdev_issue_discard() and in that case silently dropping head and tail is
not an option. Supporting non-aligned blkdev_issue_zeroout() via submitting
properly aligned blkdev_issue_discard() and then just submitting writes
with zeros for head & tail would look useful to me.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 0/2] Fix blkdev_issue_discard()

[Hannes Reinecke]

On 12/16/2015 09:44 AM, Jan Kara wrote:
> On Wed 16-12-15 00:12:23, Mike Snitzer wrote:
>> On Tue, Dec 15 2015 at  9:38pm -0500,
>> Martin K. Petersen <martin.petersen@oracle.com> wrote:
>>
>>>>>>>> "Bart" == Bart Van Assche <bart.vanassche@sandisk.com> writes:
>>>
>>> Bart> Should the caller of blkdev_issue_discard() implement this or
>>> Bart> should this functionality be added in blkdev_issue_discard()
>>> Bart> itself ?
>>>
>>> I'm very much against dropping information in the ioctl/filesystem
>>> submission path. But I am not against having a helper function of some
>>> sort that DM and target could use to handle heads and tails.
>>>
>>> Right now DM does not handle this and I think it would be worthwhile to
>>> have. Mike, what are your requirements?
>>
>> If the discard doesn't conform to the device's stacked discard limits
>> then yes the head and/or tail will get dropped on the floor.
>>
>> Not sure why DM is being made the focal point on this.  Are you thinking
>> specifically about DM-thinp and its discard_granularity?  DM thinp
>> doesn't support partial thinp block discards -- so misaligned/partial
>> discards are dropped by DM thinp.  There isn't a compelling case for
>> fixing this (by adding a bitset for each thinp block to support finer
>> grained discards) -- at least not that I'm aware of.
>>
>> But I'm missing exactly what it is your helper function would do... and
>> yet you're asking me what my requirements are.. sorry ;)
>
> I agree that just dropping head & tail from misaligned discard request
> looks reasonable. However I think the original motivation for this thread
> was actually blkdev_issue_zeroout(). That ends up being implemented using
> blkdev_issue_discard() and in that case silently dropping head and tail is
> not an option. Supporting non-aligned blkdev_issue_zeroout() via submitting
> properly aligned blkdev_issue_discard() and then just submitting writes
> with zeros for head & tail would look useful to me.
>
And we need a similar thing for SMR drives, too.
SMR drives might end up supporting discard only for a certain range 
of blocks, _and_ set 'discard_zeroes_data', too, for those ranges.
So for those we'd need a similar thing.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 0/2] Fix blkdev_issue_discard()

[Martin K. Petersen]

>>>>> "Mike" == Mike Snitzer <snitzer@redhat.com> writes:

Mike,

Mike> DM thinp doesn't support partial thinp block discards -- so
Mike> misaligned/partial discards are dropped by DM thinp.  There isn't
Mike> a compelling case for fixing this (by adding a bitset for each
Mike> thinp block to support finer grained discards) -- at least not
Mike> that I'm aware of.

Mike> But I'm missing exactly what it is your helper function would
Mike> do... and yet you're asking me what my requirements are.. sorry ;)

This came up because the SCSI target would like to support
discard_zeroes_data on top of a backing store that has a discard
granularity bigger than a logical block.

This means that the target code will have to be able to explicitly zero
out head and tail ends of a request. Since you have the same "problem"
in thinp I was wondering if it would make sense to have a common
block layer function for this?

Because the discard granularity is a hint I don't like the idea of
blkdev_issue_zeroout() having to slice and dice. I really think the
decision about what to zero and what to unmap belongs in the entity
managing the allocation units. I.e. target or thinp. But that doesn't
mean that some of the required logic could not be shared in blk-lib.c.

-- 
Martin K. Petersen	Oracle Linux Engineering