Re: [PATCH 06/16][cr][v3]: Checkpoint file-locks

[Oren Laadan <orenl@cs.columbia.edu>]

On 08/03/2010 07:11 PM, Sukadev Bhattiprolu wrote:
> While checkpointing each file-descriptor, find all the locks on the
> file and save information about the lock in the checkpoint-image.
> A follow-on patch will use this informaiton to restore the file-locks.
>
> Changelog[v3]:
> 	[Oren Laadan] Add a missing (loff_t) type cast and use a macro
> 		to set the marker/dummy file lock
>
> Changelog[v2]:
> 	[Matt Helsley]: Use fixed sizes (__s64) instead of 'loff_t' in
> 		'struct ckpt_hdr_file_lock'.
> 	[Matt Helsley, Serge Hallyn]: Highlight new use of BKL (using
> 		lock_flocks() macros as suggested by Serge).
> 	[Matt Helsley]: Reorg code a bit to simplify error handling.
> 	[Matt Helsley]: Reorg code to initialize marker-lock (Pass a
> 		NULL lock to checkpoint_one_lock() to indicate marker).
>
> Signed-off-by: Sukadev Bhattiprolu<sukadev@linux.vnet.ibm.com>
> ---
>   fs/checkpoint.c                |  100 ++++++++++++++++++++++++++++++++++-----
>   include/linux/checkpoint_hdr.h |   18 +++++++
>   2 files changed, 105 insertions(+), 13 deletions(-)
>
> diff --git a/fs/checkpoint.c b/fs/checkpoint.c
> index b5486c1..57b6944 100644
> --- a/fs/checkpoint.c
> +++ b/fs/checkpoint.c
> @@ -26,8 +26,19 @@
>   #include<linux/checkpoint.h>
>   #include<linux/eventpoll.h>
>   #include<linux/eventfd.h>
> +#include<linux/smp_lock.h>
>   #include<net/sock.h>
>
> +/*
> + * TODO: This code uses the BKL for consistency with other uses of
> + * 	 'for_each_lock()'. But since the BKL may be replaced with another
> + * 	 lock in the future, use lock_flocks() macros instead. lock_flocks()
> + * 	 are currently used in BKL-fix sand boxes and when those changes
> + * 	 are merged, the following macros can be removed
> + */
> +#define lock_flocks()		lock_kernel()
> +#define unlock_flocks()	unlock_kernel()
> +
>   /**************************************************************************
>    * Checkpoint
>    */
> @@ -256,8 +267,78 @@ static int checkpoint_file(struct ckpt_ctx *ctx, void *ptr)
>   	return ret;
>   }

I prefer the approach of writing-all-at-once over the current
one-at-a-time-and-mark-the-end. See my previous comment:
(https://lists.linux-foundation.org/pipermail/containers/2010-July/025057.html)

---
   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.
---

(Yes, I'm aware that if the list is long you may need to send
multiple chunks and for that "remember" where you were in the
list...)

[...]

Oren.