BIND 10 #2850: Define and implement ZoneTableSegmentMapped

[BIND 10 Development]

#2850: Define and implement ZoneTableSegmentMapped
-------------------------------------+-------------------------------------
            Reporter:  jinmei        |                        Owner:
                Type:  task          |  jinmei
            Priority:  medium        |                       Status:
           Component:  data source   |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130514
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  5             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |  shared memory data source
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:13 muks]:

 > Firstly, this is an incomplete set of changes. I have implemented most
 > points from your review comments, but before spending more time on this
 > ticket, I want to be sure that the design looks reasonable to you.  I
 > have to implement more detailed tests, but I want to hold off on it
 > until the interfaces and their documentation look fine to you.

 From a scan of revised reset() (with its subroutines) and its
 documentation, the overall design seems to make sense.

 Some comments I've noticed for the code, but this is far from
 comprehensive or detailed review:

 - this what() message doesn't seem to be very helpful:
 {{{#!cpp
                         "Error in resetting zone table segment to use "
 }}}
   and repeated multiple times.
 - openReadOnly: this doesn't match the content of the method:
 {{{#!cpp
     // In case there is a checksum mismatch, we throw. We want the
     // segment to be automatically destroyed then.
 }}}
   As it doesn't compare checksums.
 - The "load" is probably misleading for `reset()` because it sounds
    like loading zone data into memory (parsing zone files or iterating
    in some data source, etc).  `reset()` is basically expected to be
    non time consuming operation.
 {{{#!cpp
     /// \brief Unload the current memory store (if loaded) and load the
     /// specified one.
 }}}

 I'm not sure I should continue the rest of the review.  For now, I'm
 only answering the bigger questions (below) and giving the ticket
 back to you.

 > I am still not entirely satisfied with the branch. Some points:
 >
 > * I think the checksum should be stored separately from the segment, and
 >   there should be no possibility that the memory area for the checksum
 >   could itself influence its computation. The current code of patching 0
 >   during checksum computation is hackish and opens the possibility of
 >   broken code. Why do this when we can cleanly separate the checksum
 >   storage?

 Such an idea simply didn't occurred to me before.  I'm open to this
 point based on pros and cons considerations, but where else would we
 store it?  In a separate file?  In that case there's another types of
 issues such as maintaining two sets of files in a consistent way.  I'm
 not necessarily saying that's obviously worse than the current
 approach, but at least it doesn't seem to be obviously better.  Or do
 you have something different in your mind?

 >   Another option is to remove checksums altogether. It only checks one
 >   byte of each page for corruption, and I wonder what value there is to
 >   do this, esp. now that we have code to mutually exclude writer and
 >   reader instances. If it is to check for memory corruption, we don't
 >   checksum many other things that may get corrupted in the auth server.

 Actually, other important intent of this checksum stuff is to make
 sure that all pages are really "touched" once (in practice) the memory
 manager opens a mapped file, so b10-auth won't have to worry about
 severe page fault overhead in the case of cold start like a restart
 from rebooting the host.  The intent is documented for the method,
 although it may be easily overlooked.

 For this purpose, however, it doesn't have to be a checksum, so one
 other option is to just do a simple scan of the pages
 (add a new `MemorySegmentMapped::scan()` method instead of
 getCheckSum() and use it in `ZoneTableSegmentMapped::reset()`, maybe
 if specified so in params).

 > * It seems that most of the functionality of
 >   `ZoneTableSegmentMapped::reset()` can be abstracted out to the base
 >   class so it works for all implementations of
 >   `ZoneTableSegment`. Methods like `isWritable()`, `getHeader()`,
 >   etc. also need not be implemented outside the base `ZoneTableSegment`
 >   then. Without this, a caller would have to know about specialization
 >   details, such as not to call `reset()` on a
 >   `ZoneTableSegmentLocal`, etc.

 Like by pulling up the main part of reset() to the base class,
 delegating `openReadWrite()`, etc, to derived classes as a protected
 method?  Hmm, possibly.  I'm not necessarily opposed to that idea, and
 I see the possibility that it will help future extensions, such as
 supporting other types of shared memory.

 I don't necessarily think, however, "a caller would have to know about
 specialization details", at least in our expected usage.
 `ConfigurableClientList::resetMemorySegment()` (#2852) wouldn't have
 to care about it; if the call to reset() throws, it can simply let it
 be propagated.  b10-auth won't have to care about it either: it only
 has to call `resetMemorySegment()` when it was told to do so by
 memmgr, and memmgr ensures `reset()` will be called only for the right
 type of segments.  The memmgr has to be aware of these types of
 segments, but that's exactly what it's expected to do.

 > * A question about the design: why support `reset()`?  Is it for
 >   performance that we re-use a `ZoneTableSegment` instead of
 >   `reset()`ing during construction and simply creating another object in
 >   case the underlying storage changes?

 I don't understand what you mean here.  At least it has nothing to do
 with "performance" in my mind.  If you mean we don't need the
 `reset()` API, specifically what would you be suggesting?

-- 
Ticket URL: <http://bind10.isc.org/ticket/2850#comment:14>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development