Re: [PATCH v2] udf : skip finding mirror meta in same location with metadata.

[Jan Kara <jack@suse.cz>]

On Sat 15-10-11 17:41:46, Namjae Jeon wrote:
> If metadata duplicate flag is clear, it means there is mirror metadata
> file in same location with metadata file.  So this patch try to skip
> finding mirror partition on this case.  Mount time is improved and avoid
> unnecessarily loading mirror metadata.
> 
> Signed-off-by: Ashish Sangwan <ashishsangwan2@gmail.com>
> Signed-off-by: Namjae Jeon <linkinjeon@gmail.com>
  Thanks for the patch but I do not completely agree. The point is that if
there is error which prevents us to read metadata of "metadata FE", then we
can very well succeed with reading equivalent metadata from the "mirror
metadata FE" even when "duplicated" flag is set - metadata is never shared
according to the specification (OSTA UDF Spec, section 2.2.10, "Flags"
bullet). That is exactly what the current code tries to achieve.

Now I agree that in the majority of cases we don't need to read mirror
metadata FE since the original one is OK so there is some room for
improvement. What we could do is just read metadata FE and if it succeeds,
skip reading mirror metadata FE and try to read it only the first time
udf_get_pblock_meta25() fails to map the block from metadata FE.

								Honza
> diff --git a/fs/udf/super.c b/fs/udf/super.c
> index e58123a..f76f9e6 100644
> --- a/fs/udf/super.c
> +++ b/fs/udf/super.c
> @@ -857,28 +857,39 @@ static int udf_load_metadata_files(struct super_block *sb, int partition)
>  		mdata->s_metadata_fe = NULL;
>  	}
>  
> -	/* mirror file entry */
> -	addr.logicalBlockNum = mdata->s_mirror_file_loc;
> -	addr.partitionReferenceNum = map->s_partition_num;
> +	if (!mdata->s_dup_md_flag) {
> +		udf_info("There is mirror metadata file in same location with metadata file.");
> +		if (fe_error) {
> +			udf_err(sb, "So mirror inode efe can not found too");
> +			goto error_exit;
> +		} else {
> +			mdata->s_mirror_fe = mdata->s_metadata_fe;
> +		}
> +	} else {
>  
> -	udf_debug("Mirror metadata file location: block = %d part = %d\n",
> -		  addr.logicalBlockNum, addr.partitionReferenceNum);
> +		/* mirror file entry */
> +		addr.logicalBlockNum = mdata->s_mirror_file_loc;
> +		addr.partitionReferenceNum = map->s_partition_num;
>  
> -	mdata->s_mirror_fe = udf_iget(sb, &addr);
> +		udf_debug("Mirror metadata file location: block = %d part = %d\n",
> +			  addr.logicalBlockNum, addr.partitionReferenceNum);
>  
> -	if (mdata->s_mirror_fe == NULL) {
> -		if (fe_error) {
> -			udf_err(sb, "mirror inode efe not found and metadata inode is missing too, exiting...\n");
> -			goto error_exit;
> -		} else
> -			udf_warn(sb, "mirror inode efe not found, but metadata inode is OK\n");
> -	} else if (UDF_I(mdata->s_mirror_fe)->i_alloc_type !=
> -		 ICBTAG_FLAG_AD_SHORT) {
> -		udf_warn(sb, "mirror inode efe does not have short allocation descriptors!\n");
> -		iput(mdata->s_mirror_fe);
> -		mdata->s_mirror_fe = NULL;
> -		if (fe_error)
> -			goto error_exit;
> +		mdata->s_mirror_fe = udf_iget(sb, &addr);
> +
> +		if (mdata->s_mirror_fe == NULL) {
> +			if (fe_error) {
> +				udf_err(sb, "mirror inode efe not found and metadata inode is missing too, exiting...\n");
> +				goto error_exit;
> +			} else
> +				udf_warn(sb, "mirror inode efe not found, but metadata inode is OK\n");
> +		} else if (UDF_I(mdata->s_mirror_fe)->i_alloc_type !=
> +			 ICBTAG_FLAG_AD_SHORT) {
> +			udf_warn(sb, "mirror inode efe does not have short allocation descriptors!\n");
> +			iput(mdata->s_mirror_fe);
> +			mdata->s_mirror_fe = NULL;
> +			if (fe_error)
> +				goto error_exit;
> +		}
>  	}
>  
>  	/*
> -- 
> 1.7.4.4
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR