On Wed, 28 Jul 2010, Sukadev Bhattiprolu wrote:
> Oren Laadan [orenl@xxxxxxxxxxxxxxx] wrote:
> >
> >
> > Sukadev Bhattiprolu wrote:
> >> Oren Laadan [orenl@xxxxxxxxxxxxxxx] wrote:
> >> | | | > + if (lock) {
> >> | > + h->fl_start = lock->fl_start;
> >> | > + h->fl_end = lock->fl_end;
> >> | > + h->fl_type = lock->fl_type;
> >> | > + h->fl_flags = lock->fl_flags;
> >> | > + } else {
> >> | > + /* Checkpoint a dummy lock as a marker */
> >> | > + h->fl_start = -1;
> >> | | Maybe designate some constant for this ? e.g. CKPT_FLOCK_NONE ?
> >> | In any case, you need a (loff_t) -1 (like in the restore code).
> >>
> >> Ok. Defining macros CKPT_HDR_SET_MARKER_LOCK() and
> >> CKPT_HDR_CHECK_MARKER_LOCK(). Also added the missing (loff_t).
> >
> > The nice thing about #define CKPT_FLOCK_NONE -1 - see also
> > the CKPT_PID_NULL, when defined in checkpoint_hdr.h, is that
> > they are intentionally visible to userspace too.
>
> The problem is marker lock is currently identified by two fields:
>
> .fl_start = -1;
> .fl_type = FL_POSIX.
>
> so CKPT_FLOCK_NONE appears misleading if it only refers to one field.
>
> Should I say CKPT_FLOCK_NONE_START and CKPT_FLOCK_NONE_TYPE ?
>
> Or, can we make CKPT_CHECK_MARKER_LOCK() and CKPT_SET_MARKER_LOCK()
> available to user space ?
Having looked at the code again - how about the following:
get rid of this "last entry" altogether; instead, during
checkpoint, first count the locks, then write a header with
the number of locks, following by that many records of the
locks themselves.
This is what we do for other resource lists/chains as well.
Also makes it a bit easier to parse since you always know
what to expect once you see the header.
Oren.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
