Re: [PATCH 1/2] btrfs: set last dir index to the current last index when opening dir

[Filipe Manana <fdmanana@kernel.org>]

On Sat, Sep 09, 2023 at 01:08:31PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> When opening a directory for reading it, we set the last index where we
> stop iteration to the value in struct btrfs_inode::index_cnt. That value
> does not match the index of the most recently added directory entry but
> it's instead the index number that will be assigned the next directory
> entry.
> 
> This means that if after the call to opendir(3) new directory entries are
> added, a readdir(3) call will return the first new directory entry. This
> is fine because POSIX says the following [1]:
> 
>   "If a file is removed from or added to the directory after the most
>    recent call to opendir() or rewinddir(), whether a subsequent call to
>    readdir() returns an entry for that file is unspecified."
> 
> For example for the test script from commit 9b378f6ad48c ("btrfs: fix
> infinite directory reads"), where we have 2000 files in a directory, ext4
> doesn't return any new directory entry after opendir(3), while xfs returns
> the first 13 new directory entries added after the opendir(3) call.
> 
> If we move to a shorter example with an empty directory when opendir(3) is
> called, and 2 files added to the directory after the opendir(3) call, then
> readdir(3) on btrfs will return the first file, ext4 and xfs return the 2
> files (but in a different order). A test program for this, reported by
> Ian Johnson, is the following:
> 
>    #include <dirent.h>
>    #include <stdio.h>
> 
>    int main(void) {
>      DIR *dir = opendir("test");
> 
>      FILE *file;
>      file = fopen("test/1", "w");
>      fwrite("1", 1, 1, file);
>      fclose(file);
> 
>      file = fopen("test/2", "w");
>      fwrite("2", 1, 1, file);
>      fclose(file);
> 
>      struct dirent *entry;
>      while ((entry = readdir(dir))) {
>         printf("%s\n", entry->d_name);
>      }
>      closedir(dir);
>      return 0;
>    }
> 
> To make this less odd, change the behaviour to never return new entries
> that were added after the opendir(3) call. This is done by setting the
> last_index field of the struct btrfs_file_private attached to the
> directory's file handle with a value matching btrfs_inode::index_cnt
> minus 1, since that value always matches the index of the next new
> directory entry and not the index of the most recently added entry.
> 
> [1] https://pubs.opengroup.org/onlinepubs/007904875/functions/readdir_r.html
> 

Also (see the linked thread):

Reported-by: Ian Johnson <ian@ianjohnson.dev>
Tested-by: Ian Johnson <ian@ianjohnson.dev>

> Link: https://lore.kernel.org/linux-btrfs/YR1P0S.NGASEG570GJ8@ianjohnson.dev/
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>  fs/btrfs/inode.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index ca0f4781b0e5..df035211bdf0 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -5782,7 +5782,8 @@ static int btrfs_get_dir_last_index(struct btrfs_inode *dir, u64 *index)
>  		}
>  	}
>  
> -	*index = dir->index_cnt;
> +	/* index_cnt is the index number of next new entry, so decrement it. */
> +	*index = dir->index_cnt - 1;
>  
>  	return 0;
>  }
> -- 
> 2.40.1
>