Message-Id: <78178FB1-8B01-4956-BDBE-751251E2D58A@dilger.ca>
Date:   Fri, 18 Nov 2022 12:46:13 -0700
From:   Andreas Dilger <adilger@...ger.ca>
To:     "Ritesh Harjani (IBM)" <ritesh.list@...il.com>
Cc:     Theodore Ts'o <tytso@....edu>, linux-ext4@...r.kernel.org,
        Harshad Shirwadkar <harshadshirwadkar@...il.com>,
        Wang Shilong <wshilong@....com>,
        Andreas Dilger <adilger.kernel@...ger.ca>,
        Li Xi <lixi@....com>,
        Saranya Muruganandam <saranyamohan@...gle.com>
Subject: Re: [RFCv1 12/72] libext2fs: dupfs: Add fs clone & merge api

On Nov 7, 2022, at 05:23, Ritesh Harjani (IBM) <ritesh.list@...il.com> wrote:
> 
> From: Saranya Muruganandam <saranyamohan@...gle.com>
> 
> This patch mainly adds "parent" & "clone_flags" member in ext2_filsys struct
> for enabling multi-threading. Based on what CLONE flags will be passed from
> the client of libext2fs down to ext2fs_clone_fs(), those structures/bitmaps will
> be cloned (thread-aware child copy) and rest will be shared with the parent fs.
> 
> The same flags will also help to merge those cloned bitmap structures back into
> the parent bitmaps when ext2fs_merge_fs() will be called with childfs struct.
> 
> Signed-off-by: Saranya Muruganandam <saranyamohan@...gle.com>
> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@...il.com>
> ---
> lib/ext2fs/dupfs.c  | 183 ++++++++++++++++++++++++++++++++++++++++++++
> lib/ext2fs/ext2fs.h |  23 ++++++
> 2 files changed, 206 insertions(+)
> 
> diff --git a/lib/ext2fs/dupfs.c b/lib/ext2fs/dupfs.c
> index 02721e1a..ecc57cf7 100644
> --- a/lib/ext2fs/dupfs.c
> +++ b/lib/ext2fs/dupfs.c
> @@ -14,8 +14,12 @@
> #if HAVE_UNISTD_H
> #include <unistd.h>
> #endif
> +#if HAVE_PTHREAD_H
> +#include <pthread.h>
> +#endif
> #include <time.h>
> #include <string.h>
> +#include <assert.h>
> 
> #include "ext2_fs.h"
> #include "ext2fsP.h"
> @@ -120,3 +124,182 @@ errout:
> 
> }
> 
> +#ifdef HAVE_PTHREAD
> +errcode_t ext2fs_clone_fs(ext2_filsys fs, ext2_filsys *dest, unsigned int flags)
> +{
> +    errcode_t retval;
> +    ext2_filsys childfs;
> +
> +    EXT2_CHECK_MAGIC(fs, EXT2_ET_MAGIC_EXT2FS_FILSYS);
> +
> +    retval = ext2fs_get_mem(sizeof(struct struct_ext2_filsys), &childfs);
> +    if (retval)
> +        return retval;
> +
> +    /* make an exact copy implying lists and memory structures are shared */
> +    memcpy(childfs, fs, sizeof(struct struct_ext2_filsys));
> +    childfs->inode_map = NULL;
> +    childfs->block_map = NULL;
> +    childfs->badblocks = NULL;
> +    childfs->dblist = NULL;
> +
> +    pthread_mutex_lock(&fs->refcount_mutex);
> +    fs->refcount++;
> +    pthread_mutex_unlock(&fs->refcount_mutex);

The locking her doesn't make sense. Why is the mutex only protecting the "refcount",
and not the rest of the fields in fs?

It would make more sense to hold the mutex over the whole copy process, from up before
the memcpy(), until down after the last structure is cloned. Otherwise, once fs has
been cloned once and given to another thread it would be possible for them to
change these structures. 

> +    if ((flags & EXT2FS_CLONE_INODE) && fs->inode_map) {
> +        retval = ext2fs_copy_bitmap(fs->inode_map, &childfs->inode_map);
> +        if (retval)
> +            return retval;
> +        childfs->inode_map->fs = childfs;
> +    }
> +
> +    if ((flags & EXT2FS_CLONE_BLOCK) && fs->block_map) {
> +        retval = ext2fs_copy_bitmap(fs->block_map, &childfs->block_map);
> +        if (retval)
> +            return retval;
> +        childfs->block_map->fs = childfs;
> +    }
> +
> +    if ((flags & EXT2FS_CLONE_BADBLOCKS) && fs->badblocks) {
> +        retval = ext2fs_badblocks_copy(fs->badblocks, &childfs->badblocks);
> +        if (retval)
> +            return retval;
> +    }
> +
> +    if ((flags & EXT2FS_CLONE_DBLIST) && fs->dblist) {
> +        retval = ext2fs_copy_dblist(fs->dblist, &childfs->dblist);
> +        if (retval)
> +            return retval;
> +        childfs->dblist->fs = childfs;
> +    }
> +
> +    /* icache when NULL will be rebuilt if needed */
> +    childfs->icache = NULL;

This should be up where the other NULL values are set. 

> +
> +    childfs->clone_flags = flags;
> +    childfs->parent = fs;
> +    *dest = childfs;
> +
> +    return 0;
> +}
> +
> +errcode_t ext2fs_merge_fs(ext2_filsys *thread_fs)
> +{
> +    ext2_filsys fs = *thread_fs;
> +    errcode_t retval = 0;
> +    ext2_filsys dest = fs->parent;
> +    ext2_filsys src = fs;
> +    unsigned int flags = fs->clone_flags;
> +    struct ext2_inode_cache *icache;
> +    io_channel dest_io;
> +    io_channel dest_image_io;
> +    ext2fs_inode_bitmap inode_map;
> +    ext2fs_block_bitmap block_map;
> +    ext2_badblocks_list badblocks;
> +    ext2_dblist dblist;
> +    void *priv_data;
> +    int fsflags;
> +
> +    pthread_mutex_lock(&fs->refcount_mutex);
> +    fs->refcount--;
> +    assert(fs->refcount >= 0);
> +    pthread_mutex_unlock(&fs->refcount_mutex);

Same here. The mutex should be held over the whole copy process. 

Cheers, Andreas


> +    icache = dest->icache;
> +    dest_io = dest->io;
> +    dest_image_io = dest->image_io;
> +    inode_map = dest->inode_map;
> +    block_map = dest->block_map;
> +    badblocks = dest->badblocks;
> +    dblist = dest->dblist;
> +    priv_data = dest->priv_data;
> +    fsflags = dest->flags;
> +
> +    memcpy(dest, src, sizeof(struct struct_ext2_filsys));
> +
> +    dest->io = dest_io;
> +    dest->image_io = dest_image_io;
> +    dest->icache = icache;
> +    dest->inode_map = inode_map;
> +    dest->block_map = block_map;
> +    dest->badblocks = badblocks;
> +    dest->dblist = dblist;
> +    dest->priv_data = priv_data;
> +    if (dest->dblist)
> +        dest->dblist->fs = dest;
> +    dest->flags = src->flags | fsflags;
> +    if (!(src->flags & EXT2_FLAG_VALID) || !(dest->flags & EXT2_FLAG_VALID))
> +        ext2fs_unmark_valid(dest);
> +
> +    if ((flags & EXT2FS_CLONE_INODE) && src->inode_map) {
> +        if (dest->inode_map == NULL) {
> +            dest->inode_map = src->inode_map;
> +            src->inode_map = NULL;
> +        } else {
> +            retval = ext2fs_merge_bitmap(src->inode_map, dest->inode_map, NULL, NULL);
> +            if (retval)
> +                goto out;
> +        }
> +        dest->inode_map->fs = dest;
> +    }
> +
> +    if ((flags & EXT2FS_CLONE_BLOCK) && src->block_map) {
> +        if (dest->block_map == NULL) {
> +            dest->block_map = src->block_map;
> +            src->block_map = NULL;
> +        } else {
> +            retval = ext2fs_merge_bitmap(src->block_map, dest->block_map, NULL, NULL);
> +            if (retval)
> +                goto out;
> +        }
> +        dest->block_map->fs = dest;
> +    }
> +
> +    if ((flags & EXT2FS_CLONE_BADBLOCKS) && src->badblocks) {
> +        if (dest->badblocks == NULL)
> +            retval = ext2fs_badblocks_copy(src->badblocks, &dest->badblocks);
> +        else
> +            retval = ext2fs_badblocks_merge(src->badblocks, dest->badblocks);
> +        if (retval)
> +            goto out;
> +    }
> +
> +    if ((flags & EXT2FS_CLONE_DBLIST) && src->dblist) {
> +        if (dest->dblist == NULL) {
> +            dest->dblist = src->dblist;
> +            src->dblist = NULL;
> +        } else {
> +            retval = ext2fs_merge_dblist(src->dblist, dest->dblist);
> +            if (retval)
> +                goto out;
> +        }
> +        dest->dblist->fs = dest;
> +    }
> +
> +    if (src->icache) {
> +        ext2fs_free_inode_cache(src->icache);
> +        src->icache = NULL;
> +    }
> +
> +out:
> +    if (src->io)
> +        io_channel_close(src->io);
> +
> +    if ((flags & EXT2FS_CLONE_INODE) && src->inode_map)
> +        ext2fs_free_generic_bmap(src->inode_map);
> +    if ((flags & EXT2FS_CLONE_BLOCK) && src->block_map)
> +        ext2fs_free_generic_bmap(src->block_map);
> +    if ((flags & EXT2FS_CLONE_BADBLOCKS) && src->badblocks)
> +        ext2fs_badblocks_list_free(src->badblocks);
> +    if ((flags & EXT2FS_CLONE_DBLIST) && src->dblist) {
> +        ext2fs_free_dblist(src->dblist);
> +        src->dblist = NULL;
> +    }
> +
> +    ext2fs_free_mem(&src);
> +    *thread_fs = NULL;
> +
> +    return retval;
> +}
> +#endif
> diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
> index 139a25fc..b1505f95 100644
> --- a/lib/ext2fs/ext2fs.h
> +++ b/lib/ext2fs/ext2fs.h
> @@ -12,6 +12,10 @@
> #ifndef _EXT2FS_EXT2FS_H
> #define _EXT2FS_EXT2FS_H
> 
> +#ifdef HAVE_PTHREAD_H
> +#include <pthread.h>
> +#endif
> +
> #ifdef __GNUC__
> #define EXT2FS_ATTR(x) __attribute__(x)
> #else
> @@ -331,6 +335,13 @@ struct struct_ext2_filsys {
>    struct ext2fs_hashmap* block_sha_map;
> 
>    const struct ext2fs_nls_table *encoding;
> +
> +#ifdef HAVE_PTHREAD
> +    struct struct_ext2_filsys *parent;
> +    size_t refcount;
> +    pthread_mutex_t refcount_mutex;
> +    unsigned int clone_flags;
> +#endif
> };
> 
> #if EXT2_FLAT_INCLUDES
> @@ -1057,6 +1068,18 @@ extern errcode_t ext2fs_move_blocks(ext2_filsys fs,
> /* check_desc.c */
> extern errcode_t ext2fs_check_desc(ext2_filsys fs);
> 
> +#ifdef HAVE_PTHREAD
> +/* flags for ext2fs_clone_fs */
> +#define EXT2FS_CLONE_BLOCK            0x0001
> +#define EXT2FS_CLONE_INODE            0x0002
> +#define EXT2FS_CLONE_BADBLOCKS        0x0004
> +#define EXT2FS_CLONE_DBLIST            0x0008
> +
> +extern errcode_t ext2fs_clone_fs(ext2_filsys fs, ext2_filsys *dest,
> +                                 unsigned int flags);
> +extern errcode_t ext2fs_merge_fs(ext2_filsys *fs);
> +#endif
> +
> /* closefs.c */
> extern errcode_t ext2fs_close(ext2_filsys fs);
> extern errcode_t ext2fs_close2(ext2_filsys fs, int flags);
> -- 
> 2.37.3
> 

Powered by blists - more mailing lists