[WIP PATCH 0/4] Support for UDF 1.01 and 2.60 revisions

[WIP PATCH 0/4] Support for UDF 1.01 and 2.60 revisions

[Pali Rohár]

These are my work-in-progress patches for UDF 1.01 and 2.60 revisions.
Patches are untested and UDF 2.60 support is only in R/O mode.

I'm sending them to know what do you think about it. There is missing
code for parsing of numFiles, numDirs, minUDFReadRev and minUDFWriteRev
values from UDF 1.50 VAT discs, but this could be done in other patches.

PS: If somebody is interested in UDF 1.01 specification, please let me
know and I could send it. It is missing on OSTA website.

Pali Rohár (4):
  udf: Do not access LVIDIU revision members when they are not filled
  udf: Fix reading numFiles and numDirs from UDF 2.00+ VAT discs
  udf: Fix reading minUDFReadRev and minUDFWriteRev from UDF 2.00+ VAT
    discs
  udf: Allow to read UDF 2.60 discs

 fs/udf/super.c  | 71 +++++++++++++++++++++++++++++++++++++++----------
 fs/udf/udf_sb.h | 10 ++++++-
 2 files changed, 66 insertions(+), 15 deletions(-)

-- 
2.20.1

[WIP PATCH 1/4] udf: Do not access LVIDIU revision members when they are not filled

[Pali Rohár]

minUDFReadRev, minUDFWriteRev and maxUDFWriteRev members were introduced in
UDF 1.02. Previous UDF revisions used that area for implementation specific
data. So in this case do not touch these members.

To check if LVIDIU contain revisions members, first read UDF revision from
LVD. If revision is at least 1.02 LVIDIU should contain revision members.

This change should fix mounting UDF 1.01 images in R/W mode. Kernel would
not touch, read overwrite implementation specific area of LVIDIU.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 fs/udf/super.c  | 37 ++++++++++++++++++++++++++-----------
 fs/udf/udf_sb.h |  3 +++
 2 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/fs/udf/super.c b/fs/udf/super.c
index 2d0b90800..8df6e9962 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -765,7 +765,7 @@ static int udf_check_vsd(struct super_block *sb)
 }
 
 static int udf_verify_domain_identifier(struct super_block *sb,
-					struct regid *ident, char *dname)
+					struct regid *ident, char *dname, u16 *udf_rev)
 {
 	struct domainIdentSuffix *suffix;
 
@@ -779,6 +779,8 @@ static int udf_verify_domain_identifier(struct super_block *sb,
 		goto force_ro;
 	}
 	suffix = (struct domainIdentSuffix *)ident->identSuffix;
+	if (udf_rev)
+		*udf_rev = le16_to_cpu(suffix->UDFRevision);
 	if ((suffix->domainFlags & DOMAIN_FLAGS_HARD_WRITE_PROTECT) ||
 	    (suffix->domainFlags & DOMAIN_FLAGS_SOFT_WRITE_PROTECT)) {
 		if (!sb_rdonly(sb)) {
@@ -801,7 +803,7 @@ static int udf_load_fileset(struct super_block *sb, struct fileSetDesc *fset,
 {
 	int ret;
 
-	ret = udf_verify_domain_identifier(sb, &fset->domainIdent, "file set");
+	ret = udf_verify_domain_identifier(sb, &fset->domainIdent, "file set", NULL);
 	if (ret < 0)
 		return ret;
 
@@ -1404,7 +1406,7 @@ static int udf_load_logicalvol(struct super_block *sb, sector_t block,
 	}
 
 	ret = udf_verify_domain_identifier(sb, &lvd->domainIdent,
-					   "logical volume");
+					   "logical volume", &sbi->s_lvd_udfrev);
 	if (ret)
 		goto out_bh;
 	ret = udf_sb_alloc_partition_maps(sb, le32_to_cpu(lvd->numPartitionMaps));
@@ -2055,12 +2057,19 @@ static void udf_close_lvid(struct super_block *sb)
 	mutex_lock(&sbi->s_alloc_mutex);
 	lvidiu->impIdent.identSuffix[0] = UDF_OS_CLASS_UNIX;
 	lvidiu->impIdent.identSuffix[1] = UDF_OS_ID_LINUX;
-	if (UDF_MAX_WRITE_VERSION > le16_to_cpu(lvidiu->maxUDFWriteRev))
-		lvidiu->maxUDFWriteRev = cpu_to_le16(UDF_MAX_WRITE_VERSION);
-	if (sbi->s_udfrev > le16_to_cpu(lvidiu->minUDFReadRev))
-		lvidiu->minUDFReadRev = cpu_to_le16(sbi->s_udfrev);
-	if (sbi->s_udfrev > le16_to_cpu(lvidiu->minUDFWriteRev))
-		lvidiu->minUDFWriteRev = cpu_to_le16(sbi->s_udfrev);
+
+	/* minUDFReadRev, minUDFWriteRev and maxUDFWriteRev members were
+	 * introduced in UDF 1.02. Previous UDF revisions used that area for
+	 * implementation specific data. So in this case do not touch it. */
+	if (sbi->s_lvd_udfrev >= 0x0102) {
+		if (UDF_MAX_WRITE_VERSION > le16_to_cpu(lvidiu->maxUDFWriteRev))
+			lvidiu->maxUDFWriteRev = cpu_to_le16(UDF_MAX_WRITE_VERSION);
+		if (sbi->s_udfrev > le16_to_cpu(lvidiu->minUDFReadRev))
+			lvidiu->minUDFReadRev = cpu_to_le16(sbi->s_udfrev);
+		if (sbi->s_udfrev > le16_to_cpu(lvidiu->minUDFWriteRev))
+			lvidiu->minUDFWriteRev = cpu_to_le16(sbi->s_udfrev);
+	}
+
 	if (!UDF_QUERY_FLAG(sb, UDF_FLAG_INCONSISTENT))
 		lvid->integrityType = cpu_to_le32(LVID_INTEGRITY_TYPE_CLOSE);
 
@@ -2220,8 +2229,14 @@ static int udf_fill_super(struct super_block *sb, void *options, int silent)
 			ret = -EINVAL;
 			goto error_out;
 		}
-		minUDFReadRev = le16_to_cpu(lvidiu->minUDFReadRev);
-		minUDFWriteRev = le16_to_cpu(lvidiu->minUDFWriteRev);
+
+		if (sbi->s_lvd_udfrev >= 0x0102) { /* minUDFReadRev and minUDFWriteRev were introduced in UDF 1.02 */
+			minUDFReadRev = le16_to_cpu(lvidiu->minUDFReadRev);
+			minUDFWriteRev = le16_to_cpu(lvidiu->minUDFWriteRev);
+		} else {
+			minUDFReadRev = minUDFWriteRev = sbi->s_lvd_udfrev;
+		}
+
 		if (minUDFReadRev > UDF_MAX_READ_VERSION) {
 			udf_err(sb, "minUDFReadRev=%x (max is %x)\n",
 				minUDFReadRev,
diff --git a/fs/udf/udf_sb.h b/fs/udf/udf_sb.h
index 3d83be54c..6bd0d4430 100644
--- a/fs/udf/udf_sb.h
+++ b/fs/udf/udf_sb.h
@@ -137,6 +137,9 @@ struct udf_sb_info {
 	/* Fileset Info */
 	__u16			s_serial_number;
 
+	/* LVD UDF revision filled to media at format time */
+	__u16			s_lvd_udfrev;
+
 	/* highest UDF revision we have recorded to this media */
 	__u16			s_udfrev;
 
-- 
2.20.1

[WIP PATCH 2/4] udf: Fix reading numFiles and numDirs from UDF 2.00+ VAT discs

[Pali Rohár]

These two fields are stored in VAT and override previous values stored in
LVIDIU.

This change contains only implementation for UDF 2.00+. For UDF 1.50 there
is an optional structure "Logical Volume Extended Information" which is not
implemented in this change yet.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 fs/udf/super.c  | 25 ++++++++++++++++++++++---
 fs/udf/udf_sb.h |  3 +++
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/fs/udf/super.c b/fs/udf/super.c
index 8df6e9962..e8661bf01 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -1202,6 +1202,8 @@ static int udf_load_vat(struct super_block *sb, int p_index, int type1_index)
 		map->s_type_specific.s_virtual.s_start_offset = 0;
 		map->s_type_specific.s_virtual.s_num_entries =
 			(sbi->s_vat_inode->i_size - 36) >> 2;
+		/* TODO: Add support for reading Logical Volume Extended Information (UDF 1.50 Errata, DCN 5003, 3.3.4.5.1.3) */
+		map->s_type_specific.s_virtual.s_has_additional_data = false;
 	} else if (map->s_partition_type == UDF_VIRTUAL_MAP20) {
 		vati = UDF_I(sbi->s_vat_inode);
 		if (vati->i_alloc_type != ICBTAG_FLAG_AD_IN_ICB) {
@@ -1215,6 +1217,12 @@ static int udf_load_vat(struct super_block *sb, int p_index, int type1_index)
 							vati->i_ext.i_data;
 		}
 
+		map->s_type_specific.s_virtual.s_has_additional_data =
+			true;
+		map->s_type_specific.s_virtual.s_num_files =
+			le32_to_cpu(vat20->numFiles);
+		map->s_type_specific.s_virtual.s_num_dirs =
+			le32_to_cpu(vat20->numDirs);
 		map->s_type_specific.s_virtual.s_start_offset =
 			le16_to_cpu(vat20->lengthHeader);
 		map->s_type_specific.s_virtual.s_num_entries =
@@ -2417,9 +2425,20 @@ static int udf_statfs(struct dentry *dentry, struct kstatfs *buf)
 	buf->f_blocks = sbi->s_partmaps[sbi->s_partition].s_partition_len;
 	buf->f_bfree = udf_count_free(sb);
 	buf->f_bavail = buf->f_bfree;
-	buf->f_files = (lvidiu != NULL ? (le32_to_cpu(lvidiu->numFiles) +
-					  le32_to_cpu(lvidiu->numDirs)) : 0)
-			+ buf->f_bfree;
+
+	if ((sbi->s_partmaps[sbi->s_partition].s_partition_type == UDF_VIRTUAL_MAP15 ||
+	     sbi->s_partmaps[sbi->s_partition].s_partition_type == UDF_VIRTUAL_MAP20) &&
+	     sbi->s_partmaps[sbi->s_partition].s_type_specific.s_virtual.s_has_additional_data)
+		buf->f_files = sbi->s_partmaps[sbi->s_partition].s_type_specific.s_virtual.s_num_files +
+			       sbi->s_partmaps[sbi->s_partition].s_type_specific.s_virtual.s_num_dirs +
+			       buf->f_bfree;
+	else if (lvidiu != NULL)
+		buf->f_files = le32_to_cpu(lvidiu->numFiles) +
+			       le32_to_cpu(lvidiu->numDirs) +
+			       buf->f_bfree;
+	else
+		buf->f_files = buf->f_bfree;
+
 	buf->f_ffree = buf->f_bfree;
 	buf->f_namelen = UDF_NAME_LEN;
 	buf->f_fsid.val[0] = (u32)id;
diff --git a/fs/udf/udf_sb.h b/fs/udf/udf_sb.h
index 6bd0d4430..c74abbc84 100644
--- a/fs/udf/udf_sb.h
+++ b/fs/udf/udf_sb.h
@@ -78,6 +78,9 @@ struct udf_sparing_data {
 struct udf_virtual_data {
 	__u32	s_num_entries;
 	__u16	s_start_offset;
+	bool	s_has_additional_data;
+	__u32	s_num_files;
+	__u32	s_num_dirs;
 };
 
 struct udf_bitmap {
-- 
2.20.1

[WIP PATCH 3/4] udf: Fix reading minUDFReadRev and minUDFWriteRev from UDF 2.00+ VAT discs

[Pali Rohár]

These two fields are stored in VAT and override previous values stored in
LVIDIU.

This change contains only implementation for UDF 2.00+. For UDF 1.50 there
is an optional structure "Logical Volume Extended Information" which is not
implemented in this change yet.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 fs/udf/super.c  | 11 ++++++++++-
 fs/udf/udf_sb.h |  2 ++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/fs/udf/super.c b/fs/udf/super.c
index e8661bf01..0dad63f88 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -1223,6 +1223,10 @@ static int udf_load_vat(struct super_block *sb, int p_index, int type1_index)
 			le32_to_cpu(vat20->numFiles);
 		map->s_type_specific.s_virtual.s_num_dirs =
 			le32_to_cpu(vat20->numDirs);
+		map->s_type_specific.s_virtual.s_min_udf_read_rev =
+			le16_to_cpu(vat20->minUDFReadRev);
+		map->s_type_specific.s_virtual.s_min_udf_write_rev =
+			le16_to_cpu(vat20->minUDFWriteRev);
 		map->s_type_specific.s_virtual.s_start_offset =
 			le16_to_cpu(vat20->lengthHeader);
 		map->s_type_specific.s_virtual.s_num_entries =
@@ -2238,7 +2242,12 @@ static int udf_fill_super(struct super_block *sb, void *options, int silent)
 			goto error_out;
 		}
 
-		if (sbi->s_lvd_udfrev >= 0x0102) { /* minUDFReadRev and minUDFWriteRev were introduced in UDF 1.02 */
+		if ((sbi->s_partmaps[sbi->s_partition].s_partition_type == UDF_VIRTUAL_MAP15 ||
+		     sbi->s_partmaps[sbi->s_partition].s_partition_type == UDF_VIRTUAL_MAP20) &&
+		     sbi->s_partmaps[sbi->s_partition].s_type_specific.s_virtual.s_has_additional_data) {
+			minUDFReadRev = sbi->s_partmaps[sbi->s_partition].s_type_specific.s_virtual.s_min_udf_read_rev;
+			minUDFWriteRev = sbi->s_partmaps[sbi->s_partition].s_type_specific.s_virtual.s_min_udf_write_rev;
+		} else if (sbi->s_lvd_udfrev >= 0x0102) { /* minUDFReadRev and minUDFWriteRev were introduced in UDF 1.02 */
 			minUDFReadRev = le16_to_cpu(lvidiu->minUDFReadRev);
 			minUDFWriteRev = le16_to_cpu(lvidiu->minUDFWriteRev);
 		} else {
diff --git a/fs/udf/udf_sb.h b/fs/udf/udf_sb.h
index c74abbc84..baac0357b 100644
--- a/fs/udf/udf_sb.h
+++ b/fs/udf/udf_sb.h
@@ -81,6 +81,8 @@ struct udf_virtual_data {
 	bool	s_has_additional_data;
 	__u32	s_num_files;
 	__u32	s_num_dirs;
+	__u16	s_min_udf_read_rev;
+	__u16	s_min_udf_write_rev;
 };
 
 struct udf_bitmap {
-- 
2.20.1

[WIP PATCH 4/4] udf: Allow to read UDF 2.60 discs

[Pali Rohár]

Current udf implementation now should be able to properly mount and read
images with UDF 2.60 revision in R/O mode without any problem.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
---
 fs/udf/udf_sb.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/udf/udf_sb.h b/fs/udf/udf_sb.h
index baac0357b..be8cb9f3b 100644
--- a/fs/udf/udf_sb.h
+++ b/fs/udf/udf_sb.h
@@ -6,7 +6,7 @@
 #include <linux/bitops.h>
 #include <linux/magic.h>
 
-#define UDF_MAX_READ_VERSION		0x0250
+#define UDF_MAX_READ_VERSION		0x0260
 #define UDF_MAX_WRITE_VERSION		0x0201
 
 #define UDF_FLAG_USE_EXTENDED_FE	0
-- 
2.20.1

Re: [WIP PATCH 2/4] udf: Fix reading numFiles and numDirs from UDF 2.00+ VAT discs

[Jan Kara]

On Sun 12-01-20 18:59:31, Pali Rohár wrote:
> These two fields are stored in VAT and override previous values stored in
> LVIDIU.
> 
> This change contains only implementation for UDF 2.00+. For UDF 1.50 there
> is an optional structure "Logical Volume Extended Information" which is not
> implemented in this change yet.
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>

For this and the following patch, I'd rather have the 'additional data'
like number of files, dirs, or revisions, stored in the superblock than
having them hidden in the VAT partition structure. And places that parse
corresponding on-disk structures would fill in the numbers into the
superblock.

								Honza
> ---
>  fs/udf/super.c  | 25 ++++++++++++++++++++++---
>  fs/udf/udf_sb.h |  3 +++
>  2 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/udf/super.c b/fs/udf/super.c
> index 8df6e9962..e8661bf01 100644
> --- a/fs/udf/super.c
> +++ b/fs/udf/super.c
> @@ -1202,6 +1202,8 @@ static int udf_load_vat(struct super_block *sb, int p_index, int type1_index)
>  		map->s_type_specific.s_virtual.s_start_offset = 0;
>  		map->s_type_specific.s_virtual.s_num_entries =
>  			(sbi->s_vat_inode->i_size - 36) >> 2;
> +		/* TODO: Add support for reading Logical Volume Extended Information (UDF 1.50 Errata, DCN 5003, 3.3.4.5.1.3) */
> +		map->s_type_specific.s_virtual.s_has_additional_data = false;
>  	} else if (map->s_partition_type == UDF_VIRTUAL_MAP20) {
>  		vati = UDF_I(sbi->s_vat_inode);
>  		if (vati->i_alloc_type != ICBTAG_FLAG_AD_IN_ICB) {
> @@ -1215,6 +1217,12 @@ static int udf_load_vat(struct super_block *sb, int p_index, int type1_index)
>  							vati->i_ext.i_data;
>  		}
>  
> +		map->s_type_specific.s_virtual.s_has_additional_data =
> +			true;
> +		map->s_type_specific.s_virtual.s_num_files =
> +			le32_to_cpu(vat20->numFiles);
> +		map->s_type_specific.s_virtual.s_num_dirs =
> +			le32_to_cpu(vat20->numDirs);
>  		map->s_type_specific.s_virtual.s_start_offset =
>  			le16_to_cpu(vat20->lengthHeader);
>  		map->s_type_specific.s_virtual.s_num_entries =
> @@ -2417,9 +2425,20 @@ static int udf_statfs(struct dentry *dentry, struct kstatfs *buf)
>  	buf->f_blocks = sbi->s_partmaps[sbi->s_partition].s_partition_len;
>  	buf->f_bfree = udf_count_free(sb);
>  	buf->f_bavail = buf->f_bfree;
> -	buf->f_files = (lvidiu != NULL ? (le32_to_cpu(lvidiu->numFiles) +
> -					  le32_to_cpu(lvidiu->numDirs)) : 0)
> -			+ buf->f_bfree;
> +
> +	if ((sbi->s_partmaps[sbi->s_partition].s_partition_type == UDF_VIRTUAL_MAP15 ||
> +	     sbi->s_partmaps[sbi->s_partition].s_partition_type == UDF_VIRTUAL_MAP20) &&
> +	     sbi->s_partmaps[sbi->s_partition].s_type_specific.s_virtual.s_has_additional_data)
> +		buf->f_files = sbi->s_partmaps[sbi->s_partition].s_type_specific.s_virtual.s_num_files +
> +			       sbi->s_partmaps[sbi->s_partition].s_type_specific.s_virtual.s_num_dirs +
> +			       buf->f_bfree;
> +	else if (lvidiu != NULL)
> +		buf->f_files = le32_to_cpu(lvidiu->numFiles) +
> +			       le32_to_cpu(lvidiu->numDirs) +
> +			       buf->f_bfree;
> +	else
> +		buf->f_files = buf->f_bfree;
> +
>  	buf->f_ffree = buf->f_bfree;
>  	buf->f_namelen = UDF_NAME_LEN;
>  	buf->f_fsid.val[0] = (u32)id;
> diff --git a/fs/udf/udf_sb.h b/fs/udf/udf_sb.h
> index 6bd0d4430..c74abbc84 100644
> --- a/fs/udf/udf_sb.h
> +++ b/fs/udf/udf_sb.h
> @@ -78,6 +78,9 @@ struct udf_sparing_data {
>  struct udf_virtual_data {
>  	__u32	s_num_entries;
>  	__u16	s_start_offset;
> +	bool	s_has_additional_data;
> +	__u32	s_num_files;
> +	__u32	s_num_dirs;
>  };
>  
>  struct udf_bitmap {
> -- 
> 2.20.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [WIP PATCH 1/4] udf: Do not access LVIDIU revision members when they are not filled

[Jan Kara]

On Sun 12-01-20 18:59:30, Pali Rohár wrote:
> minUDFReadRev, minUDFWriteRev and maxUDFWriteRev members were introduced in
> UDF 1.02. Previous UDF revisions used that area for implementation specific
> data. So in this case do not touch these members.
> 
> To check if LVIDIU contain revisions members, first read UDF revision from
> LVD. If revision is at least 1.02 LVIDIU should contain revision members.
> 
> This change should fix mounting UDF 1.01 images in R/W mode. Kernel would
> not touch, read overwrite implementation specific area of LVIDIU.
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>

Maybe we could store the fs revision in the superblock as well to avoid
passing the udf_rev parameter?

Also this patch contains several lines over 80 columns.

									Honza

> ---
>  fs/udf/super.c  | 37 ++++++++++++++++++++++++++-----------
>  fs/udf/udf_sb.h |  3 +++
>  2 files changed, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/udf/super.c b/fs/udf/super.c
> index 2d0b90800..8df6e9962 100644
> --- a/fs/udf/super.c
> +++ b/fs/udf/super.c
> @@ -765,7 +765,7 @@ static int udf_check_vsd(struct super_block *sb)
>  }
>  
>  static int udf_verify_domain_identifier(struct super_block *sb,
> -					struct regid *ident, char *dname)
> +					struct regid *ident, char *dname, u16 *udf_rev)
>  {
>  	struct domainIdentSuffix *suffix;
>  
> @@ -779,6 +779,8 @@ static int udf_verify_domain_identifier(struct super_block *sb,
>  		goto force_ro;
>  	}
>  	suffix = (struct domainIdentSuffix *)ident->identSuffix;
> +	if (udf_rev)
> +		*udf_rev = le16_to_cpu(suffix->UDFRevision);
>  	if ((suffix->domainFlags & DOMAIN_FLAGS_HARD_WRITE_PROTECT) ||
>  	    (suffix->domainFlags & DOMAIN_FLAGS_SOFT_WRITE_PROTECT)) {
>  		if (!sb_rdonly(sb)) {
> @@ -801,7 +803,7 @@ static int udf_load_fileset(struct super_block *sb, struct fileSetDesc *fset,
>  {
>  	int ret;
>  
> -	ret = udf_verify_domain_identifier(sb, &fset->domainIdent, "file set");
> +	ret = udf_verify_domain_identifier(sb, &fset->domainIdent, "file set", NULL);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -1404,7 +1406,7 @@ static int udf_load_logicalvol(struct super_block *sb, sector_t block,
>  	}
>  
>  	ret = udf_verify_domain_identifier(sb, &lvd->domainIdent,
> -					   "logical volume");
> +					   "logical volume", &sbi->s_lvd_udfrev);
>  	if (ret)
>  		goto out_bh;
>  	ret = udf_sb_alloc_partition_maps(sb, le32_to_cpu(lvd->numPartitionMaps));
> @@ -2055,12 +2057,19 @@ static void udf_close_lvid(struct super_block *sb)
>  	mutex_lock(&sbi->s_alloc_mutex);
>  	lvidiu->impIdent.identSuffix[0] = UDF_OS_CLASS_UNIX;
>  	lvidiu->impIdent.identSuffix[1] = UDF_OS_ID_LINUX;
> -	if (UDF_MAX_WRITE_VERSION > le16_to_cpu(lvidiu->maxUDFWriteRev))
> -		lvidiu->maxUDFWriteRev = cpu_to_le16(UDF_MAX_WRITE_VERSION);
> -	if (sbi->s_udfrev > le16_to_cpu(lvidiu->minUDFReadRev))
> -		lvidiu->minUDFReadRev = cpu_to_le16(sbi->s_udfrev);
> -	if (sbi->s_udfrev > le16_to_cpu(lvidiu->minUDFWriteRev))
> -		lvidiu->minUDFWriteRev = cpu_to_le16(sbi->s_udfrev);
> +
> +	/* minUDFReadRev, minUDFWriteRev and maxUDFWriteRev members were
> +	 * introduced in UDF 1.02. Previous UDF revisions used that area for
> +	 * implementation specific data. So in this case do not touch it. */
> +	if (sbi->s_lvd_udfrev >= 0x0102) {
> +		if (UDF_MAX_WRITE_VERSION > le16_to_cpu(lvidiu->maxUDFWriteRev))
> +			lvidiu->maxUDFWriteRev = cpu_to_le16(UDF_MAX_WRITE_VERSION);
> +		if (sbi->s_udfrev > le16_to_cpu(lvidiu->minUDFReadRev))
> +			lvidiu->minUDFReadRev = cpu_to_le16(sbi->s_udfrev);
> +		if (sbi->s_udfrev > le16_to_cpu(lvidiu->minUDFWriteRev))
> +			lvidiu->minUDFWriteRev = cpu_to_le16(sbi->s_udfrev);
> +	}
> +
>  	if (!UDF_QUERY_FLAG(sb, UDF_FLAG_INCONSISTENT))
>  		lvid->integrityType = cpu_to_le32(LVID_INTEGRITY_TYPE_CLOSE);
>  
> @@ -2220,8 +2229,14 @@ static int udf_fill_super(struct super_block *sb, void *options, int silent)
>  			ret = -EINVAL;
>  			goto error_out;
>  		}
> -		minUDFReadRev = le16_to_cpu(lvidiu->minUDFReadRev);
> -		minUDFWriteRev = le16_to_cpu(lvidiu->minUDFWriteRev);
> +
> +		if (sbi->s_lvd_udfrev >= 0x0102) { /* minUDFReadRev and minUDFWriteRev were introduced in UDF 1.02 */
> +			minUDFReadRev = le16_to_cpu(lvidiu->minUDFReadRev);
> +			minUDFWriteRev = le16_to_cpu(lvidiu->minUDFWriteRev);
> +		} else {
> +			minUDFReadRev = minUDFWriteRev = sbi->s_lvd_udfrev;
> +		}
> +
>  		if (minUDFReadRev > UDF_MAX_READ_VERSION) {
>  			udf_err(sb, "minUDFReadRev=%x (max is %x)\n",
>  				minUDFReadRev,
> diff --git a/fs/udf/udf_sb.h b/fs/udf/udf_sb.h
> index 3d83be54c..6bd0d4430 100644
> --- a/fs/udf/udf_sb.h
> +++ b/fs/udf/udf_sb.h
> @@ -137,6 +137,9 @@ struct udf_sb_info {
>  	/* Fileset Info */
>  	__u16			s_serial_number;
>  
> +	/* LVD UDF revision filled to media at format time */
> +	__u16			s_lvd_udfrev;
> +
>  	/* highest UDF revision we have recorded to this media */
>  	__u16			s_udfrev;
>  
> -- 
> 2.20.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [WIP PATCH 2/4] udf: Fix reading numFiles and numDirs from UDF 2.00+ VAT discs

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 4716 bytes --]

On Monday 13 January 2020 12:58:22 Jan Kara wrote:
> On Sun 12-01-20 18:59:31, Pali Rohár wrote:
> > These two fields are stored in VAT and override previous values stored in
> > LVIDIU.
> > 
> > This change contains only implementation for UDF 2.00+. For UDF 1.50 there
> > is an optional structure "Logical Volume Extended Information" which is not
> > implemented in this change yet.
> > 
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> 
> For this and the following patch, I'd rather have the 'additional data'
> like number of files, dirs, or revisions, stored in the superblock than
> having them hidden in the VAT partition structure. And places that parse
> corresponding on-disk structures would fill in the numbers into the
> superblock.

This is not simple. Kernel first reads and parses VAT and later parses
LVIDIU. VAT is optional UDF feature and in UDF 1.50 are even those data
optional.

Logic for determining minimal write UDF revision is currently in code
which parse LVIDIU. And this is the only place which needs access UDF
revisions stored in VAT and LVIDIU.

UDF revision from LVD is already stored into superblock.

And because VAT is parsed prior to parsing LVIDIU is is also not easy to
store number of files and directories into superblock. LVIDIU needs to
know if data from VAT were loaded to superblock or not and needs to know
if data from LVIDIU should be stored into superblock or not.

Any idea how to do it without complicating whole code?

> 								Honza
> > ---
> >  fs/udf/super.c  | 25 ++++++++++++++++++++++---
> >  fs/udf/udf_sb.h |  3 +++
> >  2 files changed, 25 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/udf/super.c b/fs/udf/super.c
> > index 8df6e9962..e8661bf01 100644
> > --- a/fs/udf/super.c
> > +++ b/fs/udf/super.c
> > @@ -1202,6 +1202,8 @@ static int udf_load_vat(struct super_block *sb, int p_index, int type1_index)
> >  		map->s_type_specific.s_virtual.s_start_offset = 0;
> >  		map->s_type_specific.s_virtual.s_num_entries =
> >  			(sbi->s_vat_inode->i_size - 36) >> 2;
> > +		/* TODO: Add support for reading Logical Volume Extended Information (UDF 1.50 Errata, DCN 5003, 3.3.4.5.1.3) */
> > +		map->s_type_specific.s_virtual.s_has_additional_data = false;
> >  	} else if (map->s_partition_type == UDF_VIRTUAL_MAP20) {
> >  		vati = UDF_I(sbi->s_vat_inode);
> >  		if (vati->i_alloc_type != ICBTAG_FLAG_AD_IN_ICB) {
> > @@ -1215,6 +1217,12 @@ static int udf_load_vat(struct super_block *sb, int p_index, int type1_index)
> >  							vati->i_ext.i_data;
> >  		}
> >  
> > +		map->s_type_specific.s_virtual.s_has_additional_data =
> > +			true;
> > +		map->s_type_specific.s_virtual.s_num_files =
> > +			le32_to_cpu(vat20->numFiles);
> > +		map->s_type_specific.s_virtual.s_num_dirs =
> > +			le32_to_cpu(vat20->numDirs);
> >  		map->s_type_specific.s_virtual.s_start_offset =
> >  			le16_to_cpu(vat20->lengthHeader);
> >  		map->s_type_specific.s_virtual.s_num_entries =
> > @@ -2417,9 +2425,20 @@ static int udf_statfs(struct dentry *dentry, struct kstatfs *buf)
> >  	buf->f_blocks = sbi->s_partmaps[sbi->s_partition].s_partition_len;
> >  	buf->f_bfree = udf_count_free(sb);
> >  	buf->f_bavail = buf->f_bfree;
> > -	buf->f_files = (lvidiu != NULL ? (le32_to_cpu(lvidiu->numFiles) +
> > -					  le32_to_cpu(lvidiu->numDirs)) : 0)
> > -			+ buf->f_bfree;
> > +
> > +	if ((sbi->s_partmaps[sbi->s_partition].s_partition_type == UDF_VIRTUAL_MAP15 ||
> > +	     sbi->s_partmaps[sbi->s_partition].s_partition_type == UDF_VIRTUAL_MAP20) &&
> > +	     sbi->s_partmaps[sbi->s_partition].s_type_specific.s_virtual.s_has_additional_data)
> > +		buf->f_files = sbi->s_partmaps[sbi->s_partition].s_type_specific.s_virtual.s_num_files +
> > +			       sbi->s_partmaps[sbi->s_partition].s_type_specific.s_virtual.s_num_dirs +
> > +			       buf->f_bfree;
> > +	else if (lvidiu != NULL)
> > +		buf->f_files = le32_to_cpu(lvidiu->numFiles) +
> > +			       le32_to_cpu(lvidiu->numDirs) +
> > +			       buf->f_bfree;
> > +	else
> > +		buf->f_files = buf->f_bfree;
> > +
> >  	buf->f_ffree = buf->f_bfree;
> >  	buf->f_namelen = UDF_NAME_LEN;
> >  	buf->f_fsid.val[0] = (u32)id;
> > diff --git a/fs/udf/udf_sb.h b/fs/udf/udf_sb.h
> > index 6bd0d4430..c74abbc84 100644
> > --- a/fs/udf/udf_sb.h
> > +++ b/fs/udf/udf_sb.h
> > @@ -78,6 +78,9 @@ struct udf_sparing_data {
> >  struct udf_virtual_data {
> >  	__u32	s_num_entries;
> >  	__u16	s_start_offset;
> > +	bool	s_has_additional_data;
> > +	__u32	s_num_files;
> > +	__u32	s_num_dirs;
> >  };
> >  
> >  struct udf_bitmap {
> > -- 
> > 2.20.1
> > 

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [WIP PATCH 1/4] udf: Do not access LVIDIU revision members when they are not filled

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 6239 bytes --]

On Monday 13 January 2020 13:00:49 Jan Kara wrote:
> On Sun 12-01-20 18:59:30, Pali Rohár wrote:
> > minUDFReadRev, minUDFWriteRev and maxUDFWriteRev members were introduced in
> > UDF 1.02. Previous UDF revisions used that area for implementation specific
> > data. So in this case do not touch these members.
> > 
> > To check if LVIDIU contain revisions members, first read UDF revision from
> > LVD. If revision is at least 1.02 LVIDIU should contain revision members.
> > 
> > This change should fix mounting UDF 1.01 images in R/W mode. Kernel would
> > not touch, read overwrite implementation specific area of LVIDIU.
> > 
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> 
> Maybe we could store the fs revision in the superblock as well to avoid
> passing the udf_rev parameter?

Unfortunately not. Function udf_verify_domain_identifier() is called
also when parsing FSD. FSD is stored on partition map and e.g. Metadata
partition map depends on UDF revision. So it is not a good idea to
overwrite UDF revision from FSD. This is reason why I decided to use
initial UDF revision number only from LVD.

But whole stuff around UDF revision is a mess. UDF revision is stored on
these locations:

main LVD
reserve LVD
main IUVD
reserve IUVD
FSD

And optionally (when specific UDF feature is used) also on:

sparable partition map 1.50+
virtual partition map 1.50+
all sparing tables 1.50+
VAT 1.50

Plus tuple minimal read, minimal write, maximal write UDF revision is
stored on:

LVIDIU 1.02+
VAT 2.00+

VAT in 2.00+ format overrides information stored on LVIDIU.

> Also this patch contains several lines over 80 columns.

Ok, this is easy to solve.

> 									Honza
> 
> > ---
> >  fs/udf/super.c  | 37 ++++++++++++++++++++++++++-----------
> >  fs/udf/udf_sb.h |  3 +++
> >  2 files changed, 29 insertions(+), 11 deletions(-)
> > 
> > diff --git a/fs/udf/super.c b/fs/udf/super.c
> > index 2d0b90800..8df6e9962 100644
> > --- a/fs/udf/super.c
> > +++ b/fs/udf/super.c
> > @@ -765,7 +765,7 @@ static int udf_check_vsd(struct super_block *sb)
> >  }
> >  
> >  static int udf_verify_domain_identifier(struct super_block *sb,
> > -					struct regid *ident, char *dname)
> > +					struct regid *ident, char *dname, u16 *udf_rev)
> >  {
> >  	struct domainIdentSuffix *suffix;
> >  
> > @@ -779,6 +779,8 @@ static int udf_verify_domain_identifier(struct super_block *sb,
> >  		goto force_ro;
> >  	}
> >  	suffix = (struct domainIdentSuffix *)ident->identSuffix;
> > +	if (udf_rev)
> > +		*udf_rev = le16_to_cpu(suffix->UDFRevision);
> >  	if ((suffix->domainFlags & DOMAIN_FLAGS_HARD_WRITE_PROTECT) ||
> >  	    (suffix->domainFlags & DOMAIN_FLAGS_SOFT_WRITE_PROTECT)) {
> >  		if (!sb_rdonly(sb)) {
> > @@ -801,7 +803,7 @@ static int udf_load_fileset(struct super_block *sb, struct fileSetDesc *fset,
> >  {
> >  	int ret;
> >  
> > -	ret = udf_verify_domain_identifier(sb, &fset->domainIdent, "file set");
> > +	ret = udf_verify_domain_identifier(sb, &fset->domainIdent, "file set", NULL);
> >  	if (ret < 0)
> >  		return ret;
> >  
> > @@ -1404,7 +1406,7 @@ static int udf_load_logicalvol(struct super_block *sb, sector_t block,
> >  	}
> >  
> >  	ret = udf_verify_domain_identifier(sb, &lvd->domainIdent,
> > -					   "logical volume");
> > +					   "logical volume", &sbi->s_lvd_udfrev);
> >  	if (ret)
> >  		goto out_bh;
> >  	ret = udf_sb_alloc_partition_maps(sb, le32_to_cpu(lvd->numPartitionMaps));
> > @@ -2055,12 +2057,19 @@ static void udf_close_lvid(struct super_block *sb)
> >  	mutex_lock(&sbi->s_alloc_mutex);
> >  	lvidiu->impIdent.identSuffix[0] = UDF_OS_CLASS_UNIX;
> >  	lvidiu->impIdent.identSuffix[1] = UDF_OS_ID_LINUX;
> > -	if (UDF_MAX_WRITE_VERSION > le16_to_cpu(lvidiu->maxUDFWriteRev))
> > -		lvidiu->maxUDFWriteRev = cpu_to_le16(UDF_MAX_WRITE_VERSION);
> > -	if (sbi->s_udfrev > le16_to_cpu(lvidiu->minUDFReadRev))
> > -		lvidiu->minUDFReadRev = cpu_to_le16(sbi->s_udfrev);
> > -	if (sbi->s_udfrev > le16_to_cpu(lvidiu->minUDFWriteRev))
> > -		lvidiu->minUDFWriteRev = cpu_to_le16(sbi->s_udfrev);
> > +
> > +	/* minUDFReadRev, minUDFWriteRev and maxUDFWriteRev members were
> > +	 * introduced in UDF 1.02. Previous UDF revisions used that area for
> > +	 * implementation specific data. So in this case do not touch it. */
> > +	if (sbi->s_lvd_udfrev >= 0x0102) {
> > +		if (UDF_MAX_WRITE_VERSION > le16_to_cpu(lvidiu->maxUDFWriteRev))
> > +			lvidiu->maxUDFWriteRev = cpu_to_le16(UDF_MAX_WRITE_VERSION);
> > +		if (sbi->s_udfrev > le16_to_cpu(lvidiu->minUDFReadRev))
> > +			lvidiu->minUDFReadRev = cpu_to_le16(sbi->s_udfrev);
> > +		if (sbi->s_udfrev > le16_to_cpu(lvidiu->minUDFWriteRev))
> > +			lvidiu->minUDFWriteRev = cpu_to_le16(sbi->s_udfrev);
> > +	}
> > +
> >  	if (!UDF_QUERY_FLAG(sb, UDF_FLAG_INCONSISTENT))
> >  		lvid->integrityType = cpu_to_le32(LVID_INTEGRITY_TYPE_CLOSE);
> >  
> > @@ -2220,8 +2229,14 @@ static int udf_fill_super(struct super_block *sb, void *options, int silent)
> >  			ret = -EINVAL;
> >  			goto error_out;
> >  		}
> > -		minUDFReadRev = le16_to_cpu(lvidiu->minUDFReadRev);
> > -		minUDFWriteRev = le16_to_cpu(lvidiu->minUDFWriteRev);
> > +
> > +		if (sbi->s_lvd_udfrev >= 0x0102) { /* minUDFReadRev and minUDFWriteRev were introduced in UDF 1.02 */
> > +			minUDFReadRev = le16_to_cpu(lvidiu->minUDFReadRev);
> > +			minUDFWriteRev = le16_to_cpu(lvidiu->minUDFWriteRev);
> > +		} else {
> > +			minUDFReadRev = minUDFWriteRev = sbi->s_lvd_udfrev;
> > +		}
> > +
> >  		if (minUDFReadRev > UDF_MAX_READ_VERSION) {
> >  			udf_err(sb, "minUDFReadRev=%x (max is %x)\n",
> >  				minUDFReadRev,
> > diff --git a/fs/udf/udf_sb.h b/fs/udf/udf_sb.h
> > index 3d83be54c..6bd0d4430 100644
> > --- a/fs/udf/udf_sb.h
> > +++ b/fs/udf/udf_sb.h
> > @@ -137,6 +137,9 @@ struct udf_sb_info {
> >  	/* Fileset Info */
> >  	__u16			s_serial_number;
> >  
> > +	/* LVD UDF revision filled to media at format time */
> > +	__u16			s_lvd_udfrev;
> > +
> >  	/* highest UDF revision we have recorded to this media */
> >  	__u16			s_udfrev;
> >  
> > -- 
> > 2.20.1
> > 

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [WIP PATCH 2/4] udf: Fix reading numFiles and numDirs from UDF 2.00+ VAT discs

[Jan Kara]

On Mon 13-01-20 19:11:38, Pali Rohár wrote:
> On Monday 13 January 2020 12:58:22 Jan Kara wrote:
> > On Sun 12-01-20 18:59:31, Pali Rohár wrote:
> > > These two fields are stored in VAT and override previous values stored in
> > > LVIDIU.
> > > 
> > > This change contains only implementation for UDF 2.00+. For UDF 1.50 there
> > > is an optional structure "Logical Volume Extended Information" which is not
> > > implemented in this change yet.
> > > 
> > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > 
> > For this and the following patch, I'd rather have the 'additional data'
> > like number of files, dirs, or revisions, stored in the superblock than
> > having them hidden in the VAT partition structure. And places that parse
> > corresponding on-disk structures would fill in the numbers into the
> > superblock.
> 
> This is not simple. Kernel first reads and parses VAT and later parses
> LVIDIU. VAT is optional UDF feature and in UDF 1.50 are even those data
> optional.
> 
> Logic for determining minimal write UDF revision is currently in code
> which parse LVIDIU. And this is the only place which needs access UDF
> revisions stored in VAT and LVIDIU.
> 
> UDF revision from LVD is already stored into superblock.
> 
> And because VAT is parsed prior to parsing LVIDIU is is also not easy to
> store number of files and directories into superblock. LVIDIU needs to
> know if data from VAT were loaded to superblock or not and needs to know
> if data from LVIDIU should be stored into superblock or not.
> 
> Any idea how to do it without complicating whole code?

Let's take the discussion about revision storage to the thread about the
other patch. But for number of directories and files I was thinking like:

We could initialize values in the superblock to -1 (or whatever invalid
value - define a constant for it). The first place that has valid values
available (detected by the superblock having still invalid values) stores them
in the superblock. We are guaranteed to parse VAT before LVIDIU because we
need VAT to locate LVIDIU in the first place so this logic should be
reliable.

And later when using the values, we can also easily check whether we
actually have sensible values available in the first place...

								Honza

> > >  fs/udf/super.c  | 25 ++++++++++++++++++++++---
> > >  fs/udf/udf_sb.h |  3 +++
> > >  2 files changed, 25 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/fs/udf/super.c b/fs/udf/super.c
> > > index 8df6e9962..e8661bf01 100644
> > > --- a/fs/udf/super.c
> > > +++ b/fs/udf/super.c
> > > @@ -1202,6 +1202,8 @@ static int udf_load_vat(struct super_block *sb, int p_index, int type1_index)
> > >  		map->s_type_specific.s_virtual.s_start_offset = 0;
> > >  		map->s_type_specific.s_virtual.s_num_entries =
> > >  			(sbi->s_vat_inode->i_size - 36) >> 2;
> > > +		/* TODO: Add support for reading Logical Volume Extended Information (UDF 1.50 Errata, DCN 5003, 3.3.4.5.1.3) */
> > > +		map->s_type_specific.s_virtual.s_has_additional_data = false;
> > >  	} else if (map->s_partition_type == UDF_VIRTUAL_MAP20) {
> > >  		vati = UDF_I(sbi->s_vat_inode);
> > >  		if (vati->i_alloc_type != ICBTAG_FLAG_AD_IN_ICB) {
> > > @@ -1215,6 +1217,12 @@ static int udf_load_vat(struct super_block *sb, int p_index, int type1_index)
> > >  							vati->i_ext.i_data;
> > >  		}
> > >  
> > > +		map->s_type_specific.s_virtual.s_has_additional_data =
> > > +			true;
> > > +		map->s_type_specific.s_virtual.s_num_files =
> > > +			le32_to_cpu(vat20->numFiles);
> > > +		map->s_type_specific.s_virtual.s_num_dirs =
> > > +			le32_to_cpu(vat20->numDirs);
> > >  		map->s_type_specific.s_virtual.s_start_offset =
> > >  			le16_to_cpu(vat20->lengthHeader);
> > >  		map->s_type_specific.s_virtual.s_num_entries =
> > > @@ -2417,9 +2425,20 @@ static int udf_statfs(struct dentry *dentry, struct kstatfs *buf)
> > >  	buf->f_blocks = sbi->s_partmaps[sbi->s_partition].s_partition_len;
> > >  	buf->f_bfree = udf_count_free(sb);
> > >  	buf->f_bavail = buf->f_bfree;
> > > -	buf->f_files = (lvidiu != NULL ? (le32_to_cpu(lvidiu->numFiles) +
> > > -					  le32_to_cpu(lvidiu->numDirs)) : 0)
> > > -			+ buf->f_bfree;
> > > +
> > > +	if ((sbi->s_partmaps[sbi->s_partition].s_partition_type == UDF_VIRTUAL_MAP15 ||
> > > +	     sbi->s_partmaps[sbi->s_partition].s_partition_type == UDF_VIRTUAL_MAP20) &&
> > > +	     sbi->s_partmaps[sbi->s_partition].s_type_specific.s_virtual.s_has_additional_data)
> > > +		buf->f_files = sbi->s_partmaps[sbi->s_partition].s_type_specific.s_virtual.s_num_files +
> > > +			       sbi->s_partmaps[sbi->s_partition].s_type_specific.s_virtual.s_num_dirs +
> > > +			       buf->f_bfree;
> > > +	else if (lvidiu != NULL)
> > > +		buf->f_files = le32_to_cpu(lvidiu->numFiles) +
> > > +			       le32_to_cpu(lvidiu->numDirs) +
> > > +			       buf->f_bfree;
> > > +	else
> > > +		buf->f_files = buf->f_bfree;
> > > +
> > >  	buf->f_ffree = buf->f_bfree;
> > >  	buf->f_namelen = UDF_NAME_LEN;
> > >  	buf->f_fsid.val[0] = (u32)id;
> > > diff --git a/fs/udf/udf_sb.h b/fs/udf/udf_sb.h
> > > index 6bd0d4430..c74abbc84 100644
> > > --- a/fs/udf/udf_sb.h
> > > +++ b/fs/udf/udf_sb.h
> > > @@ -78,6 +78,9 @@ struct udf_sparing_data {
> > >  struct udf_virtual_data {
> > >  	__u32	s_num_entries;
> > >  	__u16	s_start_offset;
> > > +	bool	s_has_additional_data;
> > > +	__u32	s_num_files;
> > > +	__u32	s_num_dirs;
> > >  };
> > >  
> > >  struct udf_bitmap {
> > > -- 
> > > 2.20.1
> > > 
> 
> -- 
> Pali Rohár
> pali.rohar@gmail.com


-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [WIP PATCH 2/4] udf: Fix reading numFiles and numDirs from UDF 2.00+ VAT discs

[Jan Kara]

On Tue 14-01-20 12:18:17, Jan Kara wrote:
> On Mon 13-01-20 19:11:38, Pali Rohár wrote:
> > On Monday 13 January 2020 12:58:22 Jan Kara wrote:
> > > On Sun 12-01-20 18:59:31, Pali Rohár wrote:
> > > > These two fields are stored in VAT and override previous values stored in
> > > > LVIDIU.
> > > > 
> > > > This change contains only implementation for UDF 2.00+. For UDF 1.50 there
> > > > is an optional structure "Logical Volume Extended Information" which is not
> > > > implemented in this change yet.
> > > > 
> > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > > 
> > > For this and the following patch, I'd rather have the 'additional data'
> > > like number of files, dirs, or revisions, stored in the superblock than
> > > having them hidden in the VAT partition structure. And places that parse
> > > corresponding on-disk structures would fill in the numbers into the
> > > superblock.
> > 
> > This is not simple. Kernel first reads and parses VAT and later parses
> > LVIDIU. VAT is optional UDF feature and in UDF 1.50 are even those data
> > optional.
> > 
> > Logic for determining minimal write UDF revision is currently in code
> > which parse LVIDIU. And this is the only place which needs access UDF
> > revisions stored in VAT and LVIDIU.
> > 
> > UDF revision from LVD is already stored into superblock.
> > 
> > And because VAT is parsed prior to parsing LVIDIU is is also not easy to
> > store number of files and directories into superblock. LVIDIU needs to
> > know if data from VAT were loaded to superblock or not and needs to know
> > if data from LVIDIU should be stored into superblock or not.
> > 
> > Any idea how to do it without complicating whole code?
> 
> Let's take the discussion about revision storage to the thread about the
> other patch. But for number of directories and files I was thinking like:
> 
> We could initialize values in the superblock to -1 (or whatever invalid
> value - define a constant for it). The first place that has valid values
> available (detected by the superblock having still invalid values) stores them
> in the superblock. We are guaranteed to parse VAT before LVIDIU because we
> need VAT to locate LVIDIU in the first place so this logic should be
> reliable.

Hum, now checking the code, I was wrong with "we are guaranteed to parse
VAT before LVIDIU". It is rather on contrary - we need to load LVID to be
able to locate VAT. So if we added processing of numDirs and numFiles from
LVID to udf_load_logicalvolint(), we can later just override the values when
parsing VAT.

								Honza

> 
> And later when using the values, we can also easily check whether we
> actually have sensible values available in the first place...
> 
> 								Honza
> 
> > > >  fs/udf/super.c  | 25 ++++++++++++++++++++++---
> > > >  fs/udf/udf_sb.h |  3 +++
> > > >  2 files changed, 25 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/fs/udf/super.c b/fs/udf/super.c
> > > > index 8df6e9962..e8661bf01 100644
> > > > --- a/fs/udf/super.c
> > > > +++ b/fs/udf/super.c
> > > > @@ -1202,6 +1202,8 @@ static int udf_load_vat(struct super_block *sb, int p_index, int type1_index)
> > > >  		map->s_type_specific.s_virtual.s_start_offset = 0;
> > > >  		map->s_type_specific.s_virtual.s_num_entries =
> > > >  			(sbi->s_vat_inode->i_size - 36) >> 2;
> > > > +		/* TODO: Add support for reading Logical Volume Extended Information (UDF 1.50 Errata, DCN 5003, 3.3.4.5.1.3) */
> > > > +		map->s_type_specific.s_virtual.s_has_additional_data = false;
> > > >  	} else if (map->s_partition_type == UDF_VIRTUAL_MAP20) {
> > > >  		vati = UDF_I(sbi->s_vat_inode);
> > > >  		if (vati->i_alloc_type != ICBTAG_FLAG_AD_IN_ICB) {
> > > > @@ -1215,6 +1217,12 @@ static int udf_load_vat(struct super_block *sb, int p_index, int type1_index)
> > > >  							vati->i_ext.i_data;
> > > >  		}
> > > >  
> > > > +		map->s_type_specific.s_virtual.s_has_additional_data =
> > > > +			true;
> > > > +		map->s_type_specific.s_virtual.s_num_files =
> > > > +			le32_to_cpu(vat20->numFiles);
> > > > +		map->s_type_specific.s_virtual.s_num_dirs =
> > > > +			le32_to_cpu(vat20->numDirs);
> > > >  		map->s_type_specific.s_virtual.s_start_offset =
> > > >  			le16_to_cpu(vat20->lengthHeader);
> > > >  		map->s_type_specific.s_virtual.s_num_entries =
> > > > @@ -2417,9 +2425,20 @@ static int udf_statfs(struct dentry *dentry, struct kstatfs *buf)
> > > >  	buf->f_blocks = sbi->s_partmaps[sbi->s_partition].s_partition_len;
> > > >  	buf->f_bfree = udf_count_free(sb);
> > > >  	buf->f_bavail = buf->f_bfree;
> > > > -	buf->f_files = (lvidiu != NULL ? (le32_to_cpu(lvidiu->numFiles) +
> > > > -					  le32_to_cpu(lvidiu->numDirs)) : 0)
> > > > -			+ buf->f_bfree;
> > > > +
> > > > +	if ((sbi->s_partmaps[sbi->s_partition].s_partition_type == UDF_VIRTUAL_MAP15 ||
> > > > +	     sbi->s_partmaps[sbi->s_partition].s_partition_type == UDF_VIRTUAL_MAP20) &&
> > > > +	     sbi->s_partmaps[sbi->s_partition].s_type_specific.s_virtual.s_has_additional_data)
> > > > +		buf->f_files = sbi->s_partmaps[sbi->s_partition].s_type_specific.s_virtual.s_num_files +
> > > > +			       sbi->s_partmaps[sbi->s_partition].s_type_specific.s_virtual.s_num_dirs +
> > > > +			       buf->f_bfree;
> > > > +	else if (lvidiu != NULL)
> > > > +		buf->f_files = le32_to_cpu(lvidiu->numFiles) +
> > > > +			       le32_to_cpu(lvidiu->numDirs) +
> > > > +			       buf->f_bfree;
> > > > +	else
> > > > +		buf->f_files = buf->f_bfree;
> > > > +
> > > >  	buf->f_ffree = buf->f_bfree;
> > > >  	buf->f_namelen = UDF_NAME_LEN;
> > > >  	buf->f_fsid.val[0] = (u32)id;
> > > > diff --git a/fs/udf/udf_sb.h b/fs/udf/udf_sb.h
> > > > index 6bd0d4430..c74abbc84 100644
> > > > --- a/fs/udf/udf_sb.h
> > > > +++ b/fs/udf/udf_sb.h
> > > > @@ -78,6 +78,9 @@ struct udf_sparing_data {
> > > >  struct udf_virtual_data {
> > > >  	__u32	s_num_entries;
> > > >  	__u16	s_start_offset;
> > > > +	bool	s_has_additional_data;
> > > > +	__u32	s_num_files;
> > > > +	__u32	s_num_dirs;
> > > >  };
> > > >  
> > > >  struct udf_bitmap {
> > > > -- 
> > > > 2.20.1
> > > > 
> > 
> > -- 
> > Pali Rohár
> > pali.rohar@gmail.com
> 
> 
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [WIP PATCH 1/4] udf: Do not access LVIDIU revision members when they are not filled

[Jan Kara]

On Mon 13-01-20 19:37:28, Pali Rohár wrote:
> On Monday 13 January 2020 13:00:49 Jan Kara wrote:
> > On Sun 12-01-20 18:59:30, Pali Rohár wrote:
> > > minUDFReadRev, minUDFWriteRev and maxUDFWriteRev members were introduced in
> > > UDF 1.02. Previous UDF revisions used that area for implementation specific
> > > data. So in this case do not touch these members.
> > > 
> > > To check if LVIDIU contain revisions members, first read UDF revision from
> > > LVD. If revision is at least 1.02 LVIDIU should contain revision members.
> > > 
> > > This change should fix mounting UDF 1.01 images in R/W mode. Kernel would
> > > not touch, read overwrite implementation specific area of LVIDIU.
> > > 
> > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > 
> > Maybe we could store the fs revision in the superblock as well to avoid
> > passing the udf_rev parameter?
> 
> Unfortunately not. Function udf_verify_domain_identifier() is called
> also when parsing FSD. FSD is stored on partition map and e.g. Metadata
> partition map depends on UDF revision. So it is not a good idea to
> overwrite UDF revision from FSD. This is reason why I decided to use
> initial UDF revision number only from LVD.
> 
> But whole stuff around UDF revision is a mess. UDF revision is stored on
> these locations:
> 
> main LVD
> reserve LVD
> main IUVD
> reserve IUVD
> FSD
> 
> And optionally (when specific UDF feature is used) also on:
> 
> sparable partition map 1.50+
> virtual partition map 1.50+
> all sparing tables 1.50+
> VAT 1.50
> 
> Plus tuple minimal read, minimal write, maximal write UDF revision is
> stored on:
> 
> LVIDIU 1.02+
> VAT 2.00+
> 
> VAT in 2.00+ format overrides information stored on LVIDIU.

Thanks for the summary. This is indeed a mess in the standard so let's not
overcomplicate it. I agree with just taking the revision from 'main LVD'
and storing it in the superblock like you do in this patch. I'd just
slightly change your code so that extracting a revision from 'struct regid'
is a separate function and not "hidden" inside
udf_verify_domain_identifier(). There's no strong reason for combining
these two.

WRT parsing of minUDFReadRev and friends, I'd handle them similarly to
numDirs and numFiles. I'd initialize them to the version we've got from
LVD, then possibly override them in udf_load_logicalvolint(), and finally
possibly override them in udf_load_vat().

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR