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

From: Jan Kara
Date: Tue Jan 14 2020 - 06:37:12 EST

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@xxxxxxxxx>
> > >
> > > 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@xxxxxxxxx
>
>
> --
> Jan Kara <jack@xxxxxxxx>
> SUSE Labs, CR
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR