Re: [PATCH v7 06/22] liveupdate: luo_file: implement file systems callbacks

[Mike Rapoport]

On Sat, Nov 22, 2025 at 05:23:33PM -0500, Pasha Tatashin wrote:
> This patch implements the core mechanism for managing preserved
> files throughout the live update lifecycle. It provides the logic to
> invoke the file handler callbacks (preserve, unpreserve, freeze,
> unfreeze, retrieve, and finish) at the appropriate stages.
> 
> During the reboot phase, luo_file_freeze() serializes the final
> metadata for each file (handler compatible string, token, and data
> handle) into a memory region preserved by KHO. In the new kernel,
> luo_file_deserialize() reconstructs the in-memory file list from this
> data, preparing the session for retrieval.
> 
> Signed-off-by: Pasha Tatashin <pasha.tatashin@xxxxxxxxxx>

With some comments below
Reviewed-by: Mike Rapoport (Microsoft) <rppt@xxxxxxxxxx>

> ---
>  include/linux/kho/abi/luo.h      |  39 +-
>  include/linux/liveupdate.h       |  98 ++++
>  kernel/liveupdate/Makefile       |   1 +
>  kernel/liveupdate/luo_file.c     | 882 +++++++++++++++++++++++++++++++
>  kernel/liveupdate/luo_internal.h |  38 ++
>  5 files changed, 1057 insertions(+), 1 deletion(-)
>  create mode 100644 kernel/liveupdate/luo_file.c
> 

...

> +int luo_preserve_file(struct luo_file_set *file_set, u64 token, int fd)
> +{
> +	struct liveupdate_file_op_args args = {0};
> +	struct liveupdate_file_handler *fh;
> +	struct luo_file *luo_file;
> +	struct file *file;
> +	int err;
> +
> +	if (luo_token_is_used(file_set, token))
> +		return -EEXIST;
> +
> +	file = fget(fd);
> +	if (!file)
> +		return -EBADF;
> +
> +	err = luo_alloc_files_mem(file_set);
> +	if (err)
> +		goto  err_files_mem;
> +
> +	if (file_set->count == LUO_FILE_MAX) {

This can be checked before getting the file and allocating memory, can't it?

> +		err = -ENOSPC;
> +		goto err_files_mem;

The goto label should say what it does, not what the error was.

> +	}
> +
> +	err = -ENOENT;
> +	luo_list_for_each_private(fh, &luo_file_handler_list, list) {
> +		if (fh->ops->can_preserve(fh, file)) {
> +			err = 0;
> +			break;
> +		}
> +	}
> +
> +	/* err is still -ENOENT if no handler was found */
> +	if (err)
> +		goto err_files_mem;
> +
> +	luo_file = kzalloc(sizeof(*luo_file), GFP_KERNEL);
> +	if (!luo_file) {
> +		err = -ENOMEM;
> +		goto err_files_mem;
> +	}
> +
> +	luo_file->file = file;
> +	luo_file->fh = fh;
> +	luo_file->token = token;
> +	luo_file->retrieved = false;
> +	mutex_init(&luo_file->mutex);
> +
> +	args.handler = fh;
> +	args.file = file;
> +	err = fh->ops->preserve(&args);
> +	if (err)
> +		goto err_kfree;
> +
> +	luo_file->serialized_data = args.serialized_data;
> +	list_add_tail(&luo_file->list, &file_set->files_list);
> +	file_set->count++;
> +
> +	return 0;
> +
> +err_kfree:
> +	mutex_destroy(&luo_file->mutex);

Don't think we need this, luo_file is freed in the next line.

> +	kfree(luo_file);
> +err_files_mem:
> +	fput(file);
> +	luo_free_files_mem(file_set);

I'd have the error path as

err_free_luo_file:
	kfree(luo_file);
err_free_files_mem:
	luo_free_files_mem(file_set);
err_put_file:
	fput(file);

> +
> +	return err;
> +}

...

> +void luo_file_unpreserve_files(struct luo_file_set *file_set)
> +{
> +	struct luo_file *luo_file;
> +
> +	while (!list_empty(&file_set->files_list)) {

list_for_each_entry_safe_reverse()?

> +		struct liveupdate_file_op_args args = {0};
> +
> +		luo_file = list_last_entry(&file_set->files_list,
> +					   struct luo_file, list);
> +
> +		args.handler = luo_file->fh;
> +		args.file = luo_file->file;
> +		args.serialized_data = luo_file->serialized_data;
> +		luo_file->fh->ops->unpreserve(&args);
> +
> +		list_del(&luo_file->list);
> +		file_set->count--;
> +
> +		fput(luo_file->file);
> +		mutex_destroy(&luo_file->mutex);
> +		kfree(luo_file);
> +	}
> +
> +	luo_free_files_mem(file_set);
> +}

...

> +int luo_file_finish(struct luo_file_set *file_set)
> +{
> +	struct list_head *files_list = &file_set->files_list;
> +	struct luo_file *luo_file;
> +	int err;
> +
> +	if (!file_set->count)
> +		return 0;
> +
> +	list_for_each_entry(luo_file, files_list, list) {
> +		err = luo_file_can_finish_one(file_set, luo_file);
> +		if (err)
> +			return err;
> +	}
> +
> +	while (!list_empty(&file_set->files_list)) {

list_for_each_entry_safe_reverse()?

> +		luo_file = list_last_entry(&file_set->files_list,
> +					   struct luo_file, list);
> +
> +		luo_file_finish_one(file_set, luo_file);
> +
> +		if (luo_file->file)
> +			fput(luo_file->file);
> +		list_del(&luo_file->list);
> +		file_set->count--;
> +		mutex_destroy(&luo_file->mutex);
> +		kfree(luo_file);
> +	}
> +

...

> diff --git a/kernel/liveupdate/luo_internal.h b/kernel/liveupdate/luo_internal.h
> index 1292ac47eef8..c8973b543d1d 100644
> --- a/kernel/liveupdate/luo_internal.h
> +++ b/kernel/liveupdate/luo_internal.h
> @@ -40,6 +40,28 @@ static inline int luo_ucmd_respond(struct luo_ucmd *ucmd,
>   */
>  #define luo_restore_fail(__fmt, ...) panic(__fmt, ##__VA_ARGS__)
>  
> +/* Mimics list_for_each_entry() but for private list head entries */
> +#define luo_list_for_each_private(pos, head, member)				\
> +	for (struct list_head *__iter = (head)->next;				\
> +	     __iter != (head) &&						\
> +	     ({ pos = container_of(__iter, typeof(*(pos)), member); 1; });	\
> +	     __iter = __iter->next)

Ideally something like this should go to include/linux/list.h, but it can
be done later to avoid bikeshedding about the name :)

And you can reuse most of list_for_each_entry, just replace the line that
accesses __private member:

#define luo_list_for_each_private(pos, head, member)			\
	for (pos = list_first_entry(head, typeof(*pos), member);	\
	     &ACCESS_PRIVATE(pos, member) != head;			\
	     pos = list_next_entry(pos, member))

-- 
Sincerely yours,
Mike.