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

Commit Message

Filipe Manana Sept. 9, 2023, 12:08 p.m. UTC

From: Filipe Manana <[e-mail]>

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

Link: https://lore.kernel.org/linux-btrfs/[e-mail]/
Signed-off-by: Filipe Manana <[e-mail]>
---
 fs/btrfs/inode.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Filipe Manana Sept. 11, 2023, 10:40 a.m. UTC | #1

On Sat, Sep 09, 2023 at 01:08:31PM +0100, [e-mail] wrote:
> From: Filipe Manana <[e-mail]>
> 
> 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 <[e-mail]>
Tested-by: Ian Johnson <[e-mail]>

> Link: https://lore.kernel.org/linux-btrfs/[e-mail]/
> Signed-off-by: Filipe Manana <[e-mail]>
> ---
>  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
>

Patch

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;
 }