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

[NamJae Jeon]

2011/10/17 Jan Kara <jack@xxxxxxx>:
> 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@xxxxxxxxx>
>> Signed-off-by: Namjae Jeon <linkinjeon@xxxxxxxxx>
> Â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.
Thanks for your idea. Actually I and ashish are also considering to
post same suggestion with your idea.
I will make the new patch included your suggestion today.
>
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â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@xxxxxxx>
> SUSE Labs, CR
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/