Re: [PATCH v2 1/3] overlay: configure base fs mount point for running tests

Amir Goldstein
2017-02-07T13:32:53Z

On Tue, Feb 7, 2017 at 2:35 PM, Eryu Guan <eguan@redhat.com> wrote:
> On Fri, Jan 27, 2017 at 10:56:03AM +0200, Amir Goldstein wrote:
>> Instead of setting the vars TEST/SCRATCH_DEV to overlay base dirs,
>> allow setting the vars SCRATCH/TEST_BASE_MNT, to configure where
>> the base fs is mounted and where overlay dirs will be created.
>> For example:
>>
>> -export TEST_DEV=/mnt/base/test/ovl
>> -export SCRATCH_DEV=/mnt/base/scratch/ovl
>> +export TEST_BASE_MNT=/mnt/base/test
>> +export SCRATCH_BASE_MNT=/mnt/base/scratch
>
> [sorry for the late review, I came back from holiday then played with
> this patchset for a while]
>
> Hmm, I noticed that there're TEST/SCRATCH_BASE_MNT and
> TEST/SCRATCH_BASE_DIR and OVERLAY_BASE_DIR introduced. They seem a bit
> complex and can be confusing to users. I'm wondering if they can be
> simplified somehow? e.g. use TEST/SCRATCH_BASE_MNT only, no ..BASE_DIR
> vars, no OVERLAY_BASE_DIR (which seems not necessary to me, it only adds
> another level to the path structure).

I find the extra level necessary.
For example, I use the same BASE_DEV for other tests, so I would like
to isolate overlayfs tests under ovl dir.
If we were to support mkfs of BASE_DEV that would have been different
but we don't.

However, I will try to get rid of some of the variables to reduce confusion.
IMO, _BASE_DIR, which replaces the bogus TEST/SCRATCH_DEV is
important for tests that mount their own custom overlays, e.g. overlay/005.
I will try to see how this can be simplified or better explained.
Please give this another thought as well.

>
> And I gave the variables naming a second thought, I think
> TEST/SCRATCH_BASE_MNT are confusing, they're used only in overlayfs
> testing, but the names seem to imply they're generic enough and useful
> in other filesystems testing too. I'd like to name them with little
> confusion. OTOH, I agree that adding "OVERLAY_" prefix is not good
> enough either (names too long).
>
> I'm not good at naming variables.., just a random idea in my head, how
> about using OVL_ prefix? i.e.
>
> OVL_TEST/SCRATCH_BASE_MNT, a bit shorter, and indicates they're only
> useful in overlayfs testing, but OVL_ prefix introduces inconsistency
> (with OVERLAY_UPPER_DIR etc., perhaps we can change all OVERLAY_ to
> OVL_?).
>

Yes, I considered OVL_ as well. I guess it is the least worse option.
No problem to align OVL_UPPER/LOWER_DIR. Will do.