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

["Pali Rohár" <pali.rohar@gmail.com>]

[-- 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 --]