Re: [PATCH] btrfs-progs: Get the highest inode for lost+found

[Qu Wenruo <quwenruo@cn.fujitsu.com>]

At 12/20/2016 08:08 PM, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>
> root->highest_inode is not accurate at the time of creating a lost+found
> and it fails because the highest_inode+1 is already present. This could be
> because of fixes after highest_inode is set. Instead, search
> for the highest inode in the tree and use it for lost+found.
>
> This makes root->highest_inode unnecessary and hence deleted.

This is much better than recording it in root.

>
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  cmds-check.c | 46 +++++++++++++++++++++++++++-------------------
>  ctree.h      |  1 -
>  disk-io.c    |  1 -
>  3 files changed, 27 insertions(+), 21 deletions(-)
>
> diff --git a/cmds-check.c b/cmds-check.c
> index 1dba298..a55d00d 100644
> --- a/cmds-check.c
> +++ b/cmds-check.c
> @@ -2853,6 +2853,31 @@ out:
>  	return ret;
>  }
>
> +static int get_highest_inode(struct btrfs_trans_handle *trans,
> +				struct btrfs_root *root,
> +				struct btrfs_path *path,
> +				u64 *highest_ino)
> +{
> +	struct btrfs_key key, found_key;
> +	int ret;
> +
> +	btrfs_init_path(path);
> +	key.objectid = BTRFS_LAST_FREE_OBJECTID;
> +	key.offset = -1;
> +	key.type = BTRFS_INODE_ITEM_KEY;
> +	ret = btrfs_search_slot(trans, root, &key, path, -1, 1);
> +	if (ret == 1) {
> +		btrfs_item_key_to_cpu(path->nodes[0], &found_key,
> +				path->slots[0] - 1);
> +		*highest_ino = found_key.objectid;
> +		ret = 0;
> +	}

I think such search may cause problem.

If the fs uses inode_map mount option, each fs tree will have a tailing 
FREE_INO and FREE_SPACE items.

And FREE_INO/FREE_SPACE are all over LAST_FREE_OBJECTID.

         item 0 key (256 INODE_ITEM 0) itemoff 16123 itemsize 160
                 inode generation 3 transid 7 size 0 nbytes 16384
                 block group 0 mode 40755 links 1 uid 0 gid 0 rdev 0
                 sequence 0 flags 0x1(none)
         item 1 key (256 INODE_REF 256) itemoff 16111 itemsize 12
                 inode ref index 0 namelen 2 name: ..
         item 2 key (FREE_INO INODE_ITEM 0) itemoff 15951 itemsize 160
                 inode generation 0 transid 7 size 0 nbytes 0
                 block group 0 mode 100600 links 1 uid 0 gid 0 rdev 0
                 sequence 24 flags 0x0(NOCOMPRESS|PREALLOC)
         item 3 key (FREE_SPACE UNTYPED 0) itemoff 15910 itemsize 41
                 location key (FREE_INO INODE_ITEM 0)
                 cache generation 0 entries 0 bitmaps 0


In that case, such search will point to the FREE_INO slot, and always 
return -EOVERFLOW.

What about check the objectid and if it's larger than 
LAST_FREE_OBJECTID, try to search previous slot?

Other part looks good for me.

Thanks,
Qu
> +	if (*highest_ino >= BTRFS_LAST_FREE_OBJECTID)
> +		ret = -EOVERFLOW;
> +	btrfs_release_path(path);
> +	return ret;
> +}
> +
>  static int repair_inode_nlinks(struct btrfs_trans_handle *trans,
>  			       struct btrfs_root *root,
>  			       struct btrfs_path *path,
> @@ -2898,11 +2923,9 @@ static int repair_inode_nlinks(struct btrfs_trans_handle *trans,
>  	}
>
>  	if (rec->found_link == 0) {
> -		lost_found_ino = root->highest_inode;
> -		if (lost_found_ino >= BTRFS_LAST_FREE_OBJECTID) {
> -			ret = -EOVERFLOW;
> +		ret = get_highest_inode(trans, root, path, &lost_found_ino);
> +		if (ret < 0)
>  			goto out;
> -		}
>  		lost_found_ino++;
>  		ret = btrfs_mkdir(trans, root, dir_name, strlen(dir_name),
>  				  BTRFS_FIRST_FREE_OBJECTID, &lost_found_ino,
> @@ -3266,21 +3289,6 @@ static int check_inode_recs(struct btrfs_root *root,
>  	}
>
>  	/*
> -	 * We need to record the highest inode number for later 'lost+found'
> -	 * dir creation.
> -	 * We must select an ino not used/referred by any existing inode, or
> -	 * 'lost+found' ino may be a missing ino in a corrupted leaf,
> -	 * this may cause 'lost+found' dir has wrong nlinks.
> -	 */
> -	cache = last_cache_extent(inode_cache);
> -	if (cache) {
> -		node = container_of(cache, struct ptr_node, cache);
> -		rec = node->data;
> -		if (rec->ino > root->highest_inode)
> -			root->highest_inode = rec->ino;
> -	}
> -
> -	/*
>  	 * We need to repair backrefs first because we could change some of the
>  	 * errors in the inode recs.
>  	 *
> diff --git a/ctree.h b/ctree.h
> index dd02ef8..0c34ae2 100644
> --- a/ctree.h
> +++ b/ctree.h
> @@ -1177,7 +1177,6 @@ struct btrfs_root {
>
>
>  	u32 type;
> -	u64 highest_inode;
>  	u64 last_inode_alloc;
>
>  	/*
> diff --git a/disk-io.c b/disk-io.c
> index 9140a81..2a94d4f 100644
> --- a/disk-io.c
> +++ b/disk-io.c
> @@ -494,7 +494,6 @@ void btrfs_setup_root(u32 nodesize, u32 leafsize, u32 sectorsize,
>  	root->fs_info = fs_info;
>  	root->objectid = objectid;
>  	root->last_trans = 0;
> -	root->highest_inode = 0;
>  	root->last_inode_alloc = 0;
>
>  	INIT_LIST_HEAD(&root->dirty_list);
>