dm-cache invalidate_cblocks range parsing

dm-cache invalidate_cblocks range parsing

[Mears, Morgan]

If one specifies a cblock range in the dm-cache invalidate_cblocks message
(commit 65790ff919e2e07ccb4457415c11075b245d643b), the final cblock in the 
range does not get invalidated.  For example, after:

	dmsetup message cache 0 invalidate_cblocks 10-11

cblock 11 will still be in the cache.  The reason is the (begin != end)
check in process_invalidation_request() (dm-cache-target.c); were it to
change to (begin <= end), the final block would be treated like the rest.

However, parse_cblock_range() relies on the current behavior for the single
block case; if the change above were applied, an extra block would be
invalidated for each single-block invalidation request.

So: is the current behavior intentional?

--Morgan

Re: dm-cache invalidate_cblocks range parsing

[Joe Thornber]

On Sun, Nov 24, 2013 at 02:25:33AM +0000, Mears, Morgan wrote:
> If one specifies a cblock range in the dm-cache invalidate_cblocks message
> (commit 65790ff919e2e07ccb4457415c11075b245d643b), the final cblock in the 
> range does not get invalidated.  For example, after:
> 
> 	dmsetup message cache 0 invalidate_cblocks 10-11
> 
> cblock 11 will still be in the cache.  The reason is the (begin != end)
> check in process_invalidation_request() (dm-cache-target.c); were it to
> change to (begin <= end), the final block would be treated like the rest.
> 
> However, parse_cblock_range() relies on the current behavior for the single
> block case; if the change above were applied, an extra block would be
> invalidated for each single-block invalidation request.
> 
> So: is the current behavior intentional?

Yes.

Re: dm-cache invalidate_cblocks range parsing

[Joe Thornber]

On Mon, Nov 25, 2013 at 09:50:19AM +0000, Joe Thornber wrote:
> On Sun, Nov 24, 2013 at 02:25:33AM +0000, Mears, Morgan wrote:
> > If one specifies a cblock range in the dm-cache invalidate_cblocks message
> > (commit 65790ff919e2e07ccb4457415c11075b245d643b), the final cblock in the 
> > range does not get invalidated.  For example, after:
> > 
> > 	dmsetup message cache 0 invalidate_cblocks 10-11
> > 
> > cblock 11 will still be in the cache.  The reason is the (begin != end)
> > check in process_invalidation_request() (dm-cache-target.c); were it to
> > change to (begin <= end), the final block would be treated like the rest.
> > 
> > However, parse_cblock_range() relies on the current behavior for the single
> > block case; if the change above were applied, an extra block would be
> > invalidated for each single-block invalidation request.
> > 
> > So: is the current behavior intentional?
> 
> Yes.

Documented in code, but I think Documentation/device-mapper/cache.txt should also say this.

/*                                                                                                                               
 * Defines a range of cblocks, begin to (end - 1) are in the range.  end is                                                      
 * the one-past-the-end value.                                                                                                   
 */

See 

https://github.com/jthornber/device-mapper-test-suite/blob/master/lib/dmtest/tests/cache/invalidate_cblocks_tests.rb

for example uses.

Re: dm-cache invalidate_cblocks range parsing

[Alasdair G Kergon]

On Sun, Nov 24, 2013 at 02:25:33AM +0000, Mears, Morgan wrote:
> 	dmsetup message cache 0 invalidate_cblocks 10-11
> cblock 11 will still be in the cache.  

I think that should be changed.

It runs counter to the way LVM handles range specifications and seems
set to create avoidable ambiguity: everyone looking at any ranges in
dm/lvm would now have to check whether the last item in a range is
included or not in each context they encounter one as we would no longer
present a consistent position on this.

To my mind, 10-11 expands to the list of integer block labels consisting 
of 10 and 11,  (We are not talking about intervals pulled out of a
continuous number line here: 10-10.9 would have no meaning.)  I consider
blocks to be discrete objects and saying "the last numbered block listed
is always excluded" strikes me as counter-intuitive.

The compromise we reached for dm statistics to reduce the chance of
misinterpretation was to use start+number_of_items: so 10+1 in this case
where only one block was meant to be included.

Alasdair

Re: dm-cache invalidate_cblocks range parsing

[Mike Snitzer]

On Mon, Nov 25 2013 at  5:46pm -0500,
Alasdair G Kergon <agk@redhat.com> wrote:

> On Sun, Nov 24, 2013 at 02:25:33AM +0000, Mears, Morgan wrote:
> > 	dmsetup message cache 0 invalidate_cblocks 10-11
> > cblock 11 will still be in the cache.  
> 
> I think that should be changed.
> 
> It runs counter to the way LVM handles range specifications and seems
> set to create avoidable ambiguity: everyone looking at any ranges in
> dm/lvm would now have to check whether the last item in a range is
> included or not in each context they encounter one as we would no longer
> present a consistent position on this.
>
> To my mind, 10-11 expands to the list of integer block labels consisting 
> of 10 and 11,  (We are not talking about intervals pulled out of a
> continuous number line here: 10-10.9 would have no meaning.)  I consider
> blocks to be discrete objects and saying "the last numbered block listed
> is always excluded" strikes me as counter-intuitive.
> 
> The compromise we reached for dm statistics to reduce the chance of
> misinterpretation was to use start+number_of_items: so 10+1 in this case
> where only one block was meant to be included.

I'm embracing what Joe would like to use.  His preference for this
syntax isn't his alone, see:
http://gcc.gnu.org/onlinedocs/libstdc++/manual/bk01pt08ch19s02.html

As for targets having different meanings for ranges.  People already
need to pour over the table syntax of a given target if they'd like to
assemble it by hand.  Needing to understand the invalidate_cblocks
mesaage range is "one past the end" isn't asking too much.

The invalidate_cblocks interface is really intended to be used by the
cache tools, not humans.

I updated cache.txt to properly document the range syntax, see:
http://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=for-next&id=f0417b93a59e8f2

Re: dm-cache invalidate_cblocks range parsing

[Alasdair G Kergon]

None of which answers the point that some of the people seeing such a
range might well interpret it incorrectly and find the correct
interpretation surprising.  I am strongly of the view that interfaces
like these should be designed to minimise the risk of ambiguity or
misinterpretation by people who didn't read all the documentation and I
still think this interface should be changed.  The advantage of the "+"
approach is that it is unusual and more likely therefore to force people
to check the documentation rather than unknowingly making an incorrect
assumption about what "-" means.

Alasdair

Re: dm-cache invalidate_cblocks range parsing

[Alasdair G Kergon]

On Tue, Nov 26, 2013 at 11:20:47AM -0500, Mike Snitzer wrote:
> http://gcc.gnu.org/onlinedocs/libstdc++/manual/bk01pt08ch19s02.html
 
And a counter-position:

  http://en.wikipedia.org/wiki/Interval_%28mathematics%29

  Integer intervals

    The notation [a .. b] when a and b are integers, or {a .. b} , or just a ..
    b is sometimes used to indicate the interval of all integers between a and b,
    including both.

    An integer interval that has a finite lower or upper endpoint always includes that endpoint.
    Therefore, the exclusion of endpoints can be explicitly denoted by
    writing a .. b − 1 , a + 1 .. b , or a + 1 .. b − 1. Alternate-bracket
    notations like [a .. b) or [a .. b[ are rarely used for integer
    intervals.

I recall this discussion began using ".." (so should conventionally
have included both endpoints) but was switched to "-" to save a
character.

It's only the exposed message interface I'm talking about here: I've
no objection to the use of boundary-style ranges within the code.

Alasdair

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: dm-cache invalidate_cblocks range parsing

[Joe Thornber]

On Mon, Nov 25, 2013 at 10:46:18PM +0000, Alasdair G Kergon wrote:
> On Sun, Nov 24, 2013 at 02:25:33AM +0000, Mears, Morgan wrote:
> > 	dmsetup message cache 0 invalidate_cblocks 10-11
> > cblock 11 will still be in the cache.  
> 
> I think that should be changed.

It's fine as it is.  Besides, this interface is for machine use.
Changing will involve changing the test suite, and era tools.  Go and
find something worthwhile to do rather than generating more work for
those who actually write something.

- Joe