Re: [PATCH 0/9][btrfs-progs] Remove unused dirstream variable

[Goffredo Baroncelli <kreijack@inwind.it>]

On 08/02/2024 18.34, Goffredo Baroncelli wrote:
> On 08/02/2024 18.28, Goffredo Baroncelli wrote:
>> On 07/02/2024 11.16, David Sterba wrote:
>>> On Sat, Dec 30, 2023 at 12:20:54PM +0100, Goffredo Baroncelli wrote:
>>>> On 14/12/2023 17.17, David Sterba wrote:
>>>>> On Sat, Dec 09, 2023 at 07:53:20PM +0100, Goffredo Baroncelli wrote:
>>>>>> From: Goffredo Baroncelli <kreijack@inwind.it>
>>>>>>
>>>>>> For historical reason, the helpers [btrfs_]open_[file_or_]dir() work with
>>>>>> directory returning the 'fd' and a 'dirstream' variable returned by
>>>>>> opendir(3).
>>>>>>
>>>>>> If the path is a file, the 'fd' is computed from open(2) and
>>>>>> dirstream is set to NULL.
>>>>>> If the path is a directory, first the directory is opened by opendir(3), then
>>>>>> the 'fd' is computed using dirfd(3).
>>>>>> However the 'dirstream' returned by opendir(3) is left open until 'fd'
>>>>>> is not needed anymore.
>>>>>>
>>>>>> In near every case the 'dirstream' variable is not used. Only 'fd' is
>>>>>> used.
>>>>>
>>>>> As I'm reading dirfd manual page, dirfd returns the internal file
>>>>> descriptor of the dirstream and it gets closed after call to closedir().
>>>>> This means if we pass a directory and want a file descriptor then its
>>>>> lifetime matches the correspoinding DIR.
>>>>>
>>>>>> A call to close_file_or_dir() freed both 'fd' and 'dirstream'.
>>>>>>
>>>>>> Aim of this patch set is to getrid of this complexity; when the path of
>>>>>> a directory is passed, the fd is get directly using open(path, O_RDONLY):
>>>>>> so we don't need to use readdir(3) and to maintain the not used variable
>>>>>> 'dirstream'.
>>>>>
>>>>> Does this work the same way as with the dirstream?
>>>>>
>>>>
>>>> Hi David, are you interested in this patch ? I think that it is a
>>>> great simplification.
>>>
>>> Sorry, I got distracted by other things.
>>>
>>> The patchset does not apply cleanly on 6.7 so I've used 6.6.3 as the
>>> base for testing. There's one failure in the cli tests, result can be
>>> seen here https://github.com/kdave/btrfs-progs/actions/runs/7813042695/job/21311365019
>>>
>>> ====== RUN MUSTFAIL /home/runner/work/btrfs-progs/btrfs-progs/btrfs filesystem resize 1:-128M /home/runner/work/btrfs-progs/btrfs-progs/tests/test.img
>>> ERROR: not a btrfs filesystem: /home/runner/work/btrfs-progs/btrfs-progs/tests/test.img
>>> failed (expected): /home/runner/work/btrfs-progs/btrfs-progs/btrfs filesystem resize 1:-128M /home/runner/work/btrfs-progs/btrfs-progs/tests/test.img
>>> no expected error message in the output 2
>>> test failed for case 003-fi-resize-args
>>>
>>> I think it's related to the changes to dirstream, however I can't
>>> reproduce it locally, only on the github actions CI.

I found the problem, and I have an idea why you cannot reproduce the problem

The test

	btrfs filesystem resize 1:-128M /home/runner/work/btrfs-progs/btrfs-progs/tests/test.img

Expect the following error message:

	ERROR: resize works on mounted filesystems and accepts only

However if you pass a file image on a NON btrfs fs, after my patch you got the following error:

	ERROR: not a btrfs filesystem: /home/ghigo/btrfs/btrfs-progs/tests/test.img

But if you do the test on a BTRFS filesystem, you get the following error

	ERROR: not a directory: /mnt/btrfs-raid1/test.img
	ERROR: resize works on mounted filesystems and accepts only
	directories as argument. Passing file containing a btrfs image
	would resize the underlying filesystem instead of the image.


Basically, my patch rearranged the tests as:
- is a btrfs filesystem
- is a directory

where before the tests were
- is a directory
- is a btrfs filesystem


In the test 013-xxxx before failed the test "is a directory", now the test "is a btrfs filesystem" fails first.


The code fails here:

cmds/filesystem.c:
static int cmd_filesystem_resize(const struct cmd_struct *cmd,
[...]
         fd = btrfs_open_dir_fd(path);
         if (fd < 0) {
                 /* The path is a directory */
                 if (fd == -3) {
                         error(
                 "resize works on mounted filesystems and accepts only\n"
                 "directories as argument. Passing file containing a btrfs image\n"
                 "would resize the underlying filesystem instead of the image.\n");
                 }
                 return 1;
         }


However the check implemented in btrfs_open_dir_fd() are:

btrfs_open_dir_fd()
   btrfs_open_fd_2()

[...]
         if (stat(path, &st) != 0) {
                 error_on(verbose, "cannot access '%s': %m", path);
                 return -1;
         }

         if (statfs(path, &stfs) != 0) {
                 error_on(verbose, "cannot access '%s': %m", path);
                 return -1;
         }

         if (stfs.f_type != BTRFS_SUPER_MAGIC) {
                 error_on(verbose, "not a btrfs filesystem: %s", path);    // <---- NOW fail first here IF the filesystem is a not
                 return -2;						  //       btrfs
         }

         if (dir_only && !S_ISDIR(st.st_mode)) {
                 error_on(verbose, "not a directory: %s", path);          // <----- BEFORE failed here
                 return -3;
         }

         if (S_ISDIR(st.st_mode) || !read_write)
                 ret = open(path, O_RDONLY);
         else
                 ret = open(path, O_RDWR);
         if (ret < 0) {
                 error_on(verbose, "cannot access '%s': %m", path);
         }




IN order to solve the issue, I swapped the check orders; to be consistent with the previous errors.
Soon I will send you a V2 version

>>>
>>> Unlike other commands in the test, this one is called on an image and it
>>> must fail to prevent accidentaly resizing underlying filesystem.
>>
>>
>> I will rebase on 6.7 and I will try to reproduce the problem. Could you share the full script, or point me where is it ?
> 
> Ignore my latter request. I found the test code.
> 
>>
>> BR
> 

-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5