Re: [LTP] [PATCH 2/6] API: tst_device: Track minimal size per filesystem

From: Petr Vorel <pvorel@suse.cz>
To: Cyril Hrubis <chrubis@suse.cz>
Cc: Richard Palethorpe <rpalethorpe@suse.com>,
	Joerg Vehlow <joerg.vehlow@aox-tech.de>,
	linux-fsdevel@vger.kernel.org, ltp@lists.linux.it,
	automated-testing@lists.yoctoproject.org
Subject: Re: [LTP] [PATCH 2/6] API: tst_device: Track minimal size per filesystem
Date: Mon, 29 Aug 2022 13:29:16 +0200	[thread overview]
Message-ID: <YwyjDLcGzhDiWXkD@pevik> (raw)
In-Reply-To: <YwybzIVhMaCqYR/S@yuki>

Hi Cyril,

thanks a lot for a review!

...
> > +++ b/include/tst_fs.h
...
> > +unsigned int tst_min_fs_size(long f_type)
> > +{
> > +	switch (f_type) {
> > +	case 0:

> TST_ALL_FILESYSTEMS ?

Thanks for catching this. Mistake which is a result of very late Friday night coding.

> > +		return MAX(DEV_SIZE_MB_BTRFS, DEV_SIZE_MB_DEFAULT);

> I do not think that we should harcode this here. I would be for a more
> dynamic approach, i.e. add a function into the tst_supported_fs_types.c
> that loops over supported filesystems and chooses max over the minimal
> values for all supported filesystems. That way if we run on embedded
> targets the device size will be 16MB as long as btrfs-progs is not
> installed. Also that way we can easily define minimal size for xfs 300MB
> and things will work for embedded as long as xfs-progs are not
> installed.
Correct. So the value for .all_filesystems should be maximum of supported
filesystems. The only think I don't like about it that it takes some time to
check everything (mkfs.* available ... etc), but we can't avoid it.

Is it worth to cache this value (make it static in the function) so that it's not
searched more than once?

> > +	case TST_BTRFS_MAGIC:
> > +		return DEV_SIZE_MB_BTRFS;
> > +	case TST_SQUASHFS_MAGIC:
> > +		return DEV_SIZE_MB_SQUASHFS;
> > +	default:
> > +		return DEV_SIZE_MB_DEFAULT;
> > +	}
> > +}
...
> > +++ b/lib/tst_fs_type.c
> > @@ -43,6 +43,34 @@ long tst_fs_type_(void (*cleanup)(void), const char *path)
> >  	return sbuf.f_type;
> >  }

> > +long tst_fs_name_type(const char *fs)
> > +{
> > +	if (!strcmp(fs, "btrfs"))
> > +		return TST_BTRFS_MAGIC;
> > +	else if (!strcmp(fs, "exfat"))
> > +		return TST_EXFAT_MAGIC;
> > +	else if (!strcmp(fs, "ext2"))
> > +		return TST_EXT2_OLD_MAGIC;

> I'm not sure that this is a correct mapping, I think that all ext
> filesystems goes by EXT234_MAGIC these days.
OK, I'll dig into kernel's fs/ext{2,4}/
The problem is, that we still theoretically support kernel 3.0,
thus old kernels are likely using this approach and newer not.
Let's see if and when it was changed.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp