[PATCH] udf: Fix free space reporting for metadata and virtual partitions

[PATCH] udf: Fix free space reporting for metadata and virtual partitions

[Jan Kara]

Free space on filesystems with metadata or virtual partition maps
currently gets misreported. This is because these partitions are just
remapped onto underlying real partitions from which keep track of free
blocks. Take this remapping into account when counting free blocks as
well.

Reported-by: Pali Rohár <pali.rohar@gmail.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/super.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

I plan to take this patch to my tree.

diff --git a/fs/udf/super.c b/fs/udf/super.c
index 8c28e93e9b73..b89e420a4b85 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -2492,17 +2492,26 @@ static unsigned int udf_count_free_table(struct super_block *sb,
 static unsigned int udf_count_free(struct super_block *sb)
 {
 	unsigned int accum = 0;
-	struct udf_sb_info *sbi;
+	struct udf_sb_info *sbi = UDF_SB(sb);
 	struct udf_part_map *map;
+	unsigned int part = sbi->s_partition;
+	int ptype = sbi->s_partmaps[part].s_partition_type;
+
+	if (ptype == UDF_METADATA_MAP25) {
+		part = sbi->s_partmaps[part].s_type_specific.s_metadata.
+							s_phys_partition_ref;
+	} else if (ptype == UDF_VIRTUAL_MAP15 || ptype == UDF_VIRTUAL_MAP20) {
+		part = UDF_I(sbi->s_vat_inode)->i_location.
+							partitionReferenceNum;
+	}
 
-	sbi = UDF_SB(sb);
 	if (sbi->s_lvid_bh) {
 		struct logicalVolIntegrityDesc *lvid =
 			(struct logicalVolIntegrityDesc *)
 			sbi->s_lvid_bh->b_data;
-		if (le32_to_cpu(lvid->numOfPartitions) > sbi->s_partition) {
+		if (le32_to_cpu(lvid->numOfPartitions) > part) {
 			accum = le32_to_cpu(
-					lvid->freeSpaceTable[sbi->s_partition]);
+					lvid->freeSpaceTable[part]);
 			if (accum == 0xFFFFFFFF)
 				accum = 0;
 		}
@@ -2511,7 +2520,7 @@ static unsigned int udf_count_free(struct super_block *sb)
 	if (accum)
 		return accum;
 
-	map = &sbi->s_partmaps[sbi->s_partition];
+	map = &sbi->s_partmaps[part];
 	if (map->s_partition_flags & UDF_PART_FLAG_UNALLOC_BITMAP) {
 		accum += udf_count_free_bitmap(sb,
 					       map->s_uspace.s_bitmap);
-- 
2.16.4

Re: [PATCH] udf: Fix free space reporting for metadata and virtual partitions

[Pali Rohár]

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

On Wednesday 08 January 2020 13:19:19 Jan Kara wrote:
> Free space on filesystems with metadata or virtual partition maps
> currently gets misreported. This is because these partitions are just
> remapped onto underlying real partitions from which keep track of free
> blocks. Take this remapping into account when counting free blocks as
> well.
> 
> Reported-by: Pali Rohár <pali.rohar@gmail.com>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/udf/super.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> I plan to take this patch to my tree.
> 
> diff --git a/fs/udf/super.c b/fs/udf/super.c
> index 8c28e93e9b73..b89e420a4b85 100644
> --- a/fs/udf/super.c
> +++ b/fs/udf/super.c
> @@ -2492,17 +2492,26 @@ static unsigned int udf_count_free_table(struct super_block *sb,
>  static unsigned int udf_count_free(struct super_block *sb)
>  {
>  	unsigned int accum = 0;
> -	struct udf_sb_info *sbi;
> +	struct udf_sb_info *sbi = UDF_SB(sb);
>  	struct udf_part_map *map;
> +	unsigned int part = sbi->s_partition;
> +	int ptype = sbi->s_partmaps[part].s_partition_type;
> +
> +	if (ptype == UDF_METADATA_MAP25) {
> +		part = sbi->s_partmaps[part].s_type_specific.s_metadata.
> +							s_phys_partition_ref;
> +	} else if (ptype == UDF_VIRTUAL_MAP15 || ptype == UDF_VIRTUAL_MAP20) {
> +		part = UDF_I(sbi->s_vat_inode)->i_location.
> +							partitionReferenceNum;

Hello! I do not think that it make sense to report "free blocks" for
discs with Virtual partition. By definition of VAT, all blocks prior to
VAT are already "read-only" and therefore these blocks cannot be use for
writing new data by any implementation. And because VAT is stored on the
last block, in our model all blocks are "occupied".

> +	}
>  
> -	sbi = UDF_SB(sb);
>  	if (sbi->s_lvid_bh) {
>  		struct logicalVolIntegrityDesc *lvid =
>  			(struct logicalVolIntegrityDesc *)
>  			sbi->s_lvid_bh->b_data;
> -		if (le32_to_cpu(lvid->numOfPartitions) > sbi->s_partition) {
> +		if (le32_to_cpu(lvid->numOfPartitions) > part) {
>  			accum = le32_to_cpu(
> -					lvid->freeSpaceTable[sbi->s_partition]);
> +					lvid->freeSpaceTable[part]);

And in any case freeSpaceTable should not be used for discs with VAT.
And we should ignore its value for discs with VAT.

UDF 2.60 2.2.6.2: Free Space Table values be maintained ... except ...
for a virtual partition ...

And same applies for "partition with Access Type pseudo-overwritable".

>  			if (accum == 0xFFFFFFFF)
>  				accum = 0;
>  		}
> @@ -2511,7 +2520,7 @@ static unsigned int udf_count_free(struct super_block *sb)
>  	if (accum)
>  		return accum;
>  
> -	map = &sbi->s_partmaps[sbi->s_partition];
> +	map = &sbi->s_partmaps[part];
>  	if (map->s_partition_flags & UDF_PART_FLAG_UNALLOC_BITMAP) {
>  		accum += udf_count_free_bitmap(sb,
>  					       map->s_uspace.s_bitmap);

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

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

Re: [PATCH] udf: Fix free space reporting for metadata and virtual partitions

[Jan Kara]

On Wed 08-01-20 23:32:40, Pali Rohár wrote:
> On Wednesday 08 January 2020 13:19:19 Jan Kara wrote:
> > Free space on filesystems with metadata or virtual partition maps
> > currently gets misreported. This is because these partitions are just
> > remapped onto underlying real partitions from which keep track of free
> > blocks. Take this remapping into account when counting free blocks as
> > well.
> > 
> > Reported-by: Pali Rohár <pali.rohar@gmail.com>
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/udf/super.c | 19 ++++++++++++++-----
> >  1 file changed, 14 insertions(+), 5 deletions(-)
> > 
> > I plan to take this patch to my tree.
> > 
> > diff --git a/fs/udf/super.c b/fs/udf/super.c
> > index 8c28e93e9b73..b89e420a4b85 100644
> > --- a/fs/udf/super.c
> > +++ b/fs/udf/super.c
> > @@ -2492,17 +2492,26 @@ static unsigned int udf_count_free_table(struct super_block *sb,
> >  static unsigned int udf_count_free(struct super_block *sb)
> >  {
> >  	unsigned int accum = 0;
> > -	struct udf_sb_info *sbi;
> > +	struct udf_sb_info *sbi = UDF_SB(sb);
> >  	struct udf_part_map *map;
> > +	unsigned int part = sbi->s_partition;
> > +	int ptype = sbi->s_partmaps[part].s_partition_type;
> > +
> > +	if (ptype == UDF_METADATA_MAP25) {
> > +		part = sbi->s_partmaps[part].s_type_specific.s_metadata.
> > +							s_phys_partition_ref;
> > +	} else if (ptype == UDF_VIRTUAL_MAP15 || ptype == UDF_VIRTUAL_MAP20) {
> > +		part = UDF_I(sbi->s_vat_inode)->i_location.
> > +							partitionReferenceNum;
> 
> Hello! I do not think that it make sense to report "free blocks" for
> discs with Virtual partition. By definition of VAT, all blocks prior to
> VAT are already "read-only" and therefore these blocks cannot be use for
> writing new data by any implementation. And because VAT is stored on the
> last block, in our model all blocks are "occupied".

Fair enough. Let's just always return 0 for disks with VAT partition.

> > +	}
> >  
> > -	sbi = UDF_SB(sb);
> >  	if (sbi->s_lvid_bh) {
> >  		struct logicalVolIntegrityDesc *lvid =
> >  			(struct logicalVolIntegrityDesc *)
> >  			sbi->s_lvid_bh->b_data;
> > -		if (le32_to_cpu(lvid->numOfPartitions) > sbi->s_partition) {
> > +		if (le32_to_cpu(lvid->numOfPartitions) > part) {
> >  			accum = le32_to_cpu(
> > -					lvid->freeSpaceTable[sbi->s_partition]);
> > +					lvid->freeSpaceTable[part]);
> 
> And in any case freeSpaceTable should not be used for discs with VAT.
> And we should ignore its value for discs with VAT.
> 
> UDF 2.60 2.2.6.2: Free Space Table values be maintained ... except ...
> for a virtual partition ...
> 
> And same applies for "partition with Access Type pseudo-overwritable".

Well this is handled by the 'accum == 0xffffffff' condition below. So we
effectively ignore these values.

> >  			if (accum == 0xFFFFFFFF)
> >  				accum = 0;
> >  		}

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

Re: [PATCH] udf: Fix free space reporting for metadata and virtual partitions

[Pali Rohár]

On Thursday 09 January 2020 13:44:05 Jan Kara wrote:
> On Wed 08-01-20 23:32:40, Pali Rohár wrote:
> > On Wednesday 08 January 2020 13:19:19 Jan Kara wrote:
> > > Free space on filesystems with metadata or virtual partition maps
> > > currently gets misreported. This is because these partitions are just
> > > remapped onto underlying real partitions from which keep track of free
> > > blocks. Take this remapping into account when counting free blocks as
> > > well.
> > > 
> > > Reported-by: Pali Rohár <pali.rohar@gmail.com>
> > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > ---
> > >  fs/udf/super.c | 19 ++++++++++++++-----
> > >  1 file changed, 14 insertions(+), 5 deletions(-)
> > > 
> > > I plan to take this patch to my tree.
> > > 
> > > diff --git a/fs/udf/super.c b/fs/udf/super.c
> > > index 8c28e93e9b73..b89e420a4b85 100644
> > > --- a/fs/udf/super.c
> > > +++ b/fs/udf/super.c
> > > @@ -2492,17 +2492,26 @@ static unsigned int udf_count_free_table(struct super_block *sb,
> > >  static unsigned int udf_count_free(struct super_block *sb)
> > >  {
> > >  	unsigned int accum = 0;
> > > -	struct udf_sb_info *sbi;
> > > +	struct udf_sb_info *sbi = UDF_SB(sb);
> > >  	struct udf_part_map *map;
> > > +	unsigned int part = sbi->s_partition;
> > > +	int ptype = sbi->s_partmaps[part].s_partition_type;
> > > +
> > > +	if (ptype == UDF_METADATA_MAP25) {
> > > +		part = sbi->s_partmaps[part].s_type_specific.s_metadata.
> > > +							s_phys_partition_ref;
> > > +	} else if (ptype == UDF_VIRTUAL_MAP15 || ptype == UDF_VIRTUAL_MAP20) {
> > > +		part = UDF_I(sbi->s_vat_inode)->i_location.
> > > +							partitionReferenceNum;
> > 
> > Hello! I do not think that it make sense to report "free blocks" for
> > discs with Virtual partition. By definition of VAT, all blocks prior to
> > VAT are already "read-only" and therefore these blocks cannot be use for
> > writing new data by any implementation. And because VAT is stored on the
> > last block, in our model all blocks are "occupied".
> 
> Fair enough. Let's just always return 0 for disks with VAT partition.
> 
> > > +	}
> > >  
> > > -	sbi = UDF_SB(sb);
> > >  	if (sbi->s_lvid_bh) {
> > >  		struct logicalVolIntegrityDesc *lvid =
> > >  			(struct logicalVolIntegrityDesc *)
> > >  			sbi->s_lvid_bh->b_data;
> > > -		if (le32_to_cpu(lvid->numOfPartitions) > sbi->s_partition) {
> > > +		if (le32_to_cpu(lvid->numOfPartitions) > part) {
> > >  			accum = le32_to_cpu(
> > > -					lvid->freeSpaceTable[sbi->s_partition]);
> > > +					lvid->freeSpaceTable[part]);
> > 
> > And in any case freeSpaceTable should not be used for discs with VAT.
> > And we should ignore its value for discs with VAT.
> > 
> > UDF 2.60 2.2.6.2: Free Space Table values be maintained ... except ...
> > for a virtual partition ...
> > 
> > And same applies for "partition with Access Type pseudo-overwritable".
> 
> Well this is handled by the 'accum == 0xffffffff' condition below. So we
> effectively ignore these values.

Ok.

> > >  			if (accum == 0xFFFFFFFF)
> > >  				accum = 0;
> > >  		}
> 
> 								Honza

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

Re: [PATCH] udf: Fix free space reporting for metadata and virtual partitions

[Pali Rohár]

On Thursday 09 January 2020 13:56:57 Pali Rohár wrote:
> On Thursday 09 January 2020 13:44:05 Jan Kara wrote:
> > On Wed 08-01-20 23:32:40, Pali Rohár wrote:
> > > On Wednesday 08 January 2020 13:19:19 Jan Kara wrote:
> > > > Free space on filesystems with metadata or virtual partition maps
> > > > currently gets misreported. This is because these partitions are just
> > > > remapped onto underlying real partitions from which keep track of free
> > > > blocks. Take this remapping into account when counting free blocks as
> > > > well.
> > > > 
> > > > Reported-by: Pali Rohár <pali.rohar@gmail.com>
> > > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > > ---
> > > >  fs/udf/super.c | 19 ++++++++++++++-----
> > > >  1 file changed, 14 insertions(+), 5 deletions(-)
> > > > 
> > > > I plan to take this patch to my tree.
> > > > 
> > > > diff --git a/fs/udf/super.c b/fs/udf/super.c
> > > > index 8c28e93e9b73..b89e420a4b85 100644
> > > > --- a/fs/udf/super.c
> > > > +++ b/fs/udf/super.c
> > > > @@ -2492,17 +2492,26 @@ static unsigned int udf_count_free_table(struct super_block *sb,
> > > >  static unsigned int udf_count_free(struct super_block *sb)
> > > >  {
> > > >  	unsigned int accum = 0;
> > > > -	struct udf_sb_info *sbi;
> > > > +	struct udf_sb_info *sbi = UDF_SB(sb);
> > > >  	struct udf_part_map *map;
> > > > +	unsigned int part = sbi->s_partition;
> > > > +	int ptype = sbi->s_partmaps[part].s_partition_type;
> > > > +
> > > > +	if (ptype == UDF_METADATA_MAP25) {
> > > > +		part = sbi->s_partmaps[part].s_type_specific.s_metadata.
> > > > +							s_phys_partition_ref;
> > > > +	} else if (ptype == UDF_VIRTUAL_MAP15 || ptype == UDF_VIRTUAL_MAP20) {
> > > > +		part = UDF_I(sbi->s_vat_inode)->i_location.
> > > > +							partitionReferenceNum;
> > > 
> > > Hello! I do not think that it make sense to report "free blocks" for
> > > discs with Virtual partition. By definition of VAT, all blocks prior to
> > > VAT are already "read-only" and therefore these blocks cannot be use for
> > > writing new data by any implementation. And because VAT is stored on the
> > > last block, in our model all blocks are "occupied".
> > 
> > Fair enough. Let's just always return 0 for disks with VAT partition.
> > 
> > > > +	}
> > > >  
> > > > -	sbi = UDF_SB(sb);
> > > >  	if (sbi->s_lvid_bh) {
> > > >  		struct logicalVolIntegrityDesc *lvid =
> > > >  			(struct logicalVolIntegrityDesc *)
> > > >  			sbi->s_lvid_bh->b_data;
> > > > -		if (le32_to_cpu(lvid->numOfPartitions) > sbi->s_partition) {
> > > > +		if (le32_to_cpu(lvid->numOfPartitions) > part) {
> > > >  			accum = le32_to_cpu(
> > > > -					lvid->freeSpaceTable[sbi->s_partition]);
> > > > +					lvid->freeSpaceTable[part]);
> > > 
> > > And in any case freeSpaceTable should not be used for discs with VAT.
> > > And we should ignore its value for discs with VAT.
> > > 
> > > UDF 2.60 2.2.6.2: Free Space Table values be maintained ... except ...
> > > for a virtual partition ...
> > > 
> > > And same applies for "partition with Access Type pseudo-overwritable".
> > 
> > Well this is handled by the 'accum == 0xffffffff' condition below. So we
> > effectively ignore these values.
> 
> Ok.

Now I'm thinking about another scenario: UDF allows you to have two
partitions of Type1 (physical) on one volume: one with read-only access
type and one with overwritable access type.

UDF 2.60 2.2.6.2 says: For a partition with Access Type read-only, the
Free Space Table value shall be set to zero. And therefore we should
ignore it.

But current implementation for discs without Metadata partition (all
with UDF 2.01) reads free space table (only) from partition

  unsigned int part = sbi->s_partition;

So is this s_partition one with read-only or overwritable access type?

And to make it more complicated, UDF 2.60 2.2.10 requires that such discs
(with two partitions) needs to have also Metadata Partition Map.

> > > >  			if (accum == 0xFFFFFFFF)
> > > >  				accum = 0;
> > > >  		}
> > 
> > 								Honza
> 

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

Re: [PATCH] udf: Fix free space reporting for metadata and virtual partitions

[Jan Kara]

On Thu 09-01-20 14:08:37, Pali Rohár wrote:
> On Thursday 09 January 2020 13:56:57 Pali Rohár wrote:
> > On Thursday 09 January 2020 13:44:05 Jan Kara wrote:
> > > On Wed 08-01-20 23:32:40, Pali Rohár wrote:
> > > > On Wednesday 08 January 2020 13:19:19 Jan Kara wrote:
> > > > > Free space on filesystems with metadata or virtual partition maps
> > > > > currently gets misreported. This is because these partitions are just
> > > > > remapped onto underlying real partitions from which keep track of free
> > > > > blocks. Take this remapping into account when counting free blocks as
> > > > > well.
> > > > > 
> > > > > Reported-by: Pali Rohár <pali.rohar@gmail.com>
> > > > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > > > ---
> > > > >  fs/udf/super.c | 19 ++++++++++++++-----
> > > > >  1 file changed, 14 insertions(+), 5 deletions(-)
> > > > > 
> > > > > I plan to take this patch to my tree.
> > > > > 
> > > > > diff --git a/fs/udf/super.c b/fs/udf/super.c
> > > > > index 8c28e93e9b73..b89e420a4b85 100644
> > > > > --- a/fs/udf/super.c
> > > > > +++ b/fs/udf/super.c
> > > > > @@ -2492,17 +2492,26 @@ static unsigned int udf_count_free_table(struct super_block *sb,
> > > > >  static unsigned int udf_count_free(struct super_block *sb)
> > > > >  {
> > > > >  	unsigned int accum = 0;
> > > > > -	struct udf_sb_info *sbi;
> > > > > +	struct udf_sb_info *sbi = UDF_SB(sb);
> > > > >  	struct udf_part_map *map;
> > > > > +	unsigned int part = sbi->s_partition;
> > > > > +	int ptype = sbi->s_partmaps[part].s_partition_type;
> > > > > +
> > > > > +	if (ptype == UDF_METADATA_MAP25) {
> > > > > +		part = sbi->s_partmaps[part].s_type_specific.s_metadata.
> > > > > +							s_phys_partition_ref;
> > > > > +	} else if (ptype == UDF_VIRTUAL_MAP15 || ptype == UDF_VIRTUAL_MAP20) {
> > > > > +		part = UDF_I(sbi->s_vat_inode)->i_location.
> > > > > +							partitionReferenceNum;
> > > > 
> > > > Hello! I do not think that it make sense to report "free blocks" for
> > > > discs with Virtual partition. By definition of VAT, all blocks prior to
> > > > VAT are already "read-only" and therefore these blocks cannot be use for
> > > > writing new data by any implementation. And because VAT is stored on the
> > > > last block, in our model all blocks are "occupied".
> > > 
> > > Fair enough. Let's just always return 0 for disks with VAT partition.
> > > 
> > > > > +	}
> > > > >  
> > > > > -	sbi = UDF_SB(sb);
> > > > >  	if (sbi->s_lvid_bh) {
> > > > >  		struct logicalVolIntegrityDesc *lvid =
> > > > >  			(struct logicalVolIntegrityDesc *)
> > > > >  			sbi->s_lvid_bh->b_data;
> > > > > -		if (le32_to_cpu(lvid->numOfPartitions) > sbi->s_partition) {
> > > > > +		if (le32_to_cpu(lvid->numOfPartitions) > part) {
> > > > >  			accum = le32_to_cpu(
> > > > > -					lvid->freeSpaceTable[sbi->s_partition]);
> > > > > +					lvid->freeSpaceTable[part]);
> > > > 
> > > > And in any case freeSpaceTable should not be used for discs with VAT.
> > > > And we should ignore its value for discs with VAT.
> > > > 
> > > > UDF 2.60 2.2.6.2: Free Space Table values be maintained ... except ...
> > > > for a virtual partition ...
> > > > 
> > > > And same applies for "partition with Access Type pseudo-overwritable".
> > > 
> > > Well this is handled by the 'accum == 0xffffffff' condition below. So we
> > > effectively ignore these values.
> > 
> > Ok.
> 
> Now I'm thinking about another scenario: UDF allows you to have two
> partitions of Type1 (physical) on one volume: one with read-only access
> type and one with overwritable access type.
> 
> UDF 2.60 2.2.6.2 says: For a partition with Access Type read-only, the
> Free Space Table value shall be set to zero. And therefore we should
> ignore it.

That's what's going to happen (the code ends up ignoring values -1 and 0).

> But current implementation for discs without Metadata partition (all
> with UDF 2.01) reads free space table (only) from partition
> 
>   unsigned int part = sbi->s_partition;
> 
> So is this s_partition one with read-only or overwritable access type?

Well, this is the partition we've got fileset from. I presume that's going
to be on overwritable partition but who knows. Honestly, I have my doubts
we'll handle disks with two Type1 partitions correctly since I never met
such disks :) Do you have some disk image to try? :)

> And to make it more complicated, UDF 2.60 2.2.10 requires that such discs
> (with two partitions) needs to have also Metadata Partition Map.

But in this case Metadata Partition Map is presumably over the overwritable
partition so we should fine, shouldn't we?

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

Re: [PATCH] udf: Fix free space reporting for metadata and virtual partitions

[Pali Rohár]

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

On Thursday 09 January 2020 17:53:24 Jan Kara wrote:
> On Thu 09-01-20 14:08:37, Pali Rohár wrote:
> > On Thursday 09 January 2020 13:56:57 Pali Rohár wrote:
> > > On Thursday 09 January 2020 13:44:05 Jan Kara wrote:
> > > > On Wed 08-01-20 23:32:40, Pali Rohár wrote:
> > > > > On Wednesday 08 January 2020 13:19:19 Jan Kara wrote:
> > > > > > Free space on filesystems with metadata or virtual partition maps
> > > > > > currently gets misreported. This is because these partitions are just
> > > > > > remapped onto underlying real partitions from which keep track of free
> > > > > > blocks. Take this remapping into account when counting free blocks as
> > > > > > well.
> > > > > > 
> > > > > > Reported-by: Pali Rohár <pali.rohar@gmail.com>
> > > > > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > > > > ---
> > > > > >  fs/udf/super.c | 19 ++++++++++++++-----
> > > > > >  1 file changed, 14 insertions(+), 5 deletions(-)
> > > > > > 
> > > > > > I plan to take this patch to my tree.
> > > > > > 
> > > > > > diff --git a/fs/udf/super.c b/fs/udf/super.c
> > > > > > index 8c28e93e9b73..b89e420a4b85 100644
> > > > > > --- a/fs/udf/super.c
> > > > > > +++ b/fs/udf/super.c
> > > > > > @@ -2492,17 +2492,26 @@ static unsigned int udf_count_free_table(struct super_block *sb,
> > > > > >  static unsigned int udf_count_free(struct super_block *sb)
> > > > > >  {
> > > > > >  	unsigned int accum = 0;
> > > > > > -	struct udf_sb_info *sbi;
> > > > > > +	struct udf_sb_info *sbi = UDF_SB(sb);
> > > > > >  	struct udf_part_map *map;
> > > > > > +	unsigned int part = sbi->s_partition;
> > > > > > +	int ptype = sbi->s_partmaps[part].s_partition_type;
> > > > > > +
> > > > > > +	if (ptype == UDF_METADATA_MAP25) {
> > > > > > +		part = sbi->s_partmaps[part].s_type_specific.s_metadata.
> > > > > > +							s_phys_partition_ref;
> > > > > > +	} else if (ptype == UDF_VIRTUAL_MAP15 || ptype == UDF_VIRTUAL_MAP20) {
> > > > > > +		part = UDF_I(sbi->s_vat_inode)->i_location.
> > > > > > +							partitionReferenceNum;
> > > > > 
> > > > > Hello! I do not think that it make sense to report "free blocks" for
> > > > > discs with Virtual partition. By definition of VAT, all blocks prior to
> > > > > VAT are already "read-only" and therefore these blocks cannot be use for
> > > > > writing new data by any implementation. And because VAT is stored on the
> > > > > last block, in our model all blocks are "occupied".
> > > > 
> > > > Fair enough. Let's just always return 0 for disks with VAT partition.
> > > > 
> > > > > > +	}
> > > > > >  
> > > > > > -	sbi = UDF_SB(sb);
> > > > > >  	if (sbi->s_lvid_bh) {
> > > > > >  		struct logicalVolIntegrityDesc *lvid =
> > > > > >  			(struct logicalVolIntegrityDesc *)
> > > > > >  			sbi->s_lvid_bh->b_data;
> > > > > > -		if (le32_to_cpu(lvid->numOfPartitions) > sbi->s_partition) {
> > > > > > +		if (le32_to_cpu(lvid->numOfPartitions) > part) {
> > > > > >  			accum = le32_to_cpu(
> > > > > > -					lvid->freeSpaceTable[sbi->s_partition]);
> > > > > > +					lvid->freeSpaceTable[part]);
> > > > > 
> > > > > And in any case freeSpaceTable should not be used for discs with VAT.
> > > > > And we should ignore its value for discs with VAT.
> > > > > 
> > > > > UDF 2.60 2.2.6.2: Free Space Table values be maintained ... except ...
> > > > > for a virtual partition ...
> > > > > 
> > > > > And same applies for "partition with Access Type pseudo-overwritable".
> > > > 
> > > > Well this is handled by the 'accum == 0xffffffff' condition below. So we
> > > > effectively ignore these values.
> > > 
> > > Ok.
> > 
> > Now I'm thinking about another scenario: UDF allows you to have two
> > partitions of Type1 (physical) on one volume: one with read-only access
> > type and one with overwritable access type.
> > 
> > UDF 2.60 2.2.6.2 says: For a partition with Access Type read-only, the
> > Free Space Table value shall be set to zero. And therefore we should
> > ignore it.
> 
> That's what's going to happen (the code ends up ignoring values -1 and 0).

Ok, this could be enough.

> > But current implementation for discs without Metadata partition (all
> > with UDF 2.01) reads free space table (only) from partition
> > 
> >   unsigned int part = sbi->s_partition;
> > 
> > So is this s_partition one with read-only or overwritable access type?
> 
> Well, this is the partition we've got fileset from. I presume that's going
> to be on overwritable partition but who knows. Honestly, I have my doubts
> we'll handle disks with two Type1 partitions correctly since I never met
> such disks :) Do you have some disk image to try? :)

Seems that I do not have such disk image.

But if kernel does not support handling such disks then it does not make
sense to do anything with this function which reports free blocks.

> > And to make it more complicated, UDF 2.60 2.2.10 requires that such discs
> > (with two partitions) needs to have also Metadata Partition Map.
> 
> But in this case Metadata Partition Map is presumably over the overwritable
> partition so we should fine, shouldn't we?

In both cases I'm just speculating what may happen... Nothing against
this patch.

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

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