RE: [PATCH v2] staging: zcache: support multiple clients, prep forKVM and RAMster

[Dan Magenheimer]

> From: Dan Carpenter [mailto:error27@xxxxxxxxx]
> 
> On Thu, Jun 30, 2011 at 04:28:14PM -0700, Dan Magenheimer wrote:
> > Hi Dan --
> >
> > Thanks for the careful review.  You're right... some
> > of this was leftover from debugging an off-by-one error,
> > though the code as is still works.
> >
> > OTOH, there's a good chance that much of this sysfs
> > code will disappear before zcache would get promoted
> > out of staging, since it is to help those experimenting
> > with zcache to get more insight into what the underlying
> > compression/accept-reject algorithms are doing.
> >
> > So I hope you (and GregKH) are OK that another version is
> > not necessary at this time to fix these.
> 
> Off by one errors are kind of insidious.  People cut and paste them
> and they spread.  If someone adds a new list of chunks then there
> are now two examples that are correct and two which have an extra
> element, so it's 50/50 that he'll copy the right one.

True, but these are NOT off-by-one errors... they are
correct-but-slightly-ugly code snippets.  (To clarify, I said
the *ugliness* arose when debugging an off-by-one error.)

Patches always welcome, and I agree that these should be
fixed eventually, assuming the code doesn't go away completely
first.. I'm simply stating the position
that going through another test/submit cycling to fix
correct-but-slightly-ugly code which is present only to
surface information for experiments is not high on my priority
list right now... unless GregKH says he won't accept the patch.
 
> Btw, looking at it again, this seems like maybe a similar issue in
> zbud_evict_zbpg():
> 
>    516          for (i = 0; i < MAX_CHUNK; i++) {
>    517  retry_unbud_list_i:
> 
> 
> MAX_CHUNKS is NCHUNKS - 1.  Shouldn't that be i < NCHUNKS so that we
> reach the last element in the list?

No, the last element in that list is unused.  There is a comment
to that effect someplace in the code.  (These lists are keeping
track of pages with "chunks" of available space and the last
entry would have no available space so is always empty.)

Thanks again for your interest... are you using zcache?

Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/