* [PATCH 1/2] btrfs: set last dir index to the current last index when opening dir
2023-09-09 12:08 [PATCH 0/2] btrfs: updates for directory reading fdmanana
@ 2023-09-09 12:08 ` fdmanana
2023-09-11 10:40 ` Filipe Manana
2023-09-09 12:08 ` [PATCH 2/2] btrfs: refresh dir last index during a rewinddir(3) call fdmanana
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: fdmanana @ 2023-09-09 12:08 UTC (permalink / raw)
To: linux-btrfs; +Cc: ian, Filipe Manana
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
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 1/2] btrfs: set last dir index to the current last index when opening dir
2023-09-09 12:08 ` [PATCH 1/2] btrfs: set last dir index to the current last index when opening dir fdmanana
@ 2023-09-11 10:40 ` Filipe Manana
0 siblings, 0 replies; 8+ messages in thread
From: Filipe Manana @ 2023-09-11 10:40 UTC (permalink / raw)
To: linux-btrfs; +Cc: ian, Filipe Manana
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] btrfs: refresh dir last index during a rewinddir(3) call
2023-09-09 12:08 [PATCH 0/2] btrfs: updates for directory reading fdmanana
2023-09-09 12:08 ` [PATCH 1/2] btrfs: set last dir index to the current last index when opening dir fdmanana
@ 2023-09-09 12:08 ` fdmanana
2023-09-11 10:36 ` Filipe Manana
2023-09-11 17:28 ` [PATCH 0/2] btrfs: updates for directory reading David Sterba
2023-09-11 17:35 ` Josef Bacik
3 siblings, 1 reply; 8+ messages in thread
From: fdmanana @ 2023-09-09 12:08 UTC (permalink / raw)
To: linux-btrfs; +Cc: ian, Filipe Manana
From: Filipe Manana <fdmanana@suse.com>
When opening a directory we find what's the index of its last entry and
then store it in the directory's file handle private data (struct
btrfs_file_private::last_index), so that in the case new directory entries
are added to a directory after an opendir(3) call we don't end up in an
infinite loop (see commit 9b378f6ad48c ("btrfs: fix infinite directory
reads")) when calling readdir(3).
However once rewinddir(3) is called, POSIX states [1] that any new
directory entries added after the previous opendir(3) call, must be
returned by subsequent calls to readdir(3):
"The rewinddir() function shall reset the position of the directory
stream to which dirp refers to the beginning of the directory.
It shall also cause the directory stream to refer to the current
state of the corresponding directory, as a call to opendir() would
have done."
We currently don't refresh the last_index field of the struct
btrfs_file_private associated to the directory, so after a rewinddir(3)
we are not returning any new entries added after the opendir(3) call.
Fix this by finding the current last index of the directory when llseek
is called agains the directory.
This can be reproduced by the following C program provided by Ian Johnson:
#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);
rewinddir(dir);
struct dirent *entry;
while ((entry = readdir(dir))) {
printf("%s\n", entry->d_name);
}
closedir(dir);
return 0;
}
Reported-by: Ian Johnson <ian@ianjohnson.dev>
Link: https://lore.kernel.org/linux-btrfs/YR1P0S.NGASEG570GJ8@ianjohnson.dev/
Fixes: 9b378f6ad48c ("btrfs: fix infinite directory reads")
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
fs/btrfs/inode.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index df035211bdf0..006ca4cb4788 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5820,6 +5820,19 @@ static int btrfs_opendir(struct inode *inode, struct file *file)
return 0;
}
+static loff_t btrfs_dir_llseek(struct file *file, loff_t offset, int whence)
+{
+ struct btrfs_file_private *private = file->private_data;
+ int ret;
+
+ ret = btrfs_get_dir_last_index(BTRFS_I(file_inode(file)),
+ &private->last_index);
+ if (ret)
+ return ret;
+
+ return generic_file_llseek(file, offset, whence);
+}
+
struct dir_entry {
u64 ino;
u64 offset;
@@ -10893,7 +10906,7 @@ static const struct inode_operations btrfs_dir_inode_operations = {
};
static const struct file_operations btrfs_dir_file_operations = {
- .llseek = generic_file_llseek,
+ .llseek = btrfs_dir_llseek,
.read = generic_read_dir,
.iterate_shared = btrfs_real_readdir,
.open = btrfs_opendir,
--
2.40.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 2/2] btrfs: refresh dir last index during a rewinddir(3) call
2023-09-09 12:08 ` [PATCH 2/2] btrfs: refresh dir last index during a rewinddir(3) call fdmanana
@ 2023-09-11 10:36 ` Filipe Manana
0 siblings, 0 replies; 8+ messages in thread
From: Filipe Manana @ 2023-09-11 10:36 UTC (permalink / raw)
To: linux-btrfs; +Cc: ian, Filipe Manana
On Sat, Sep 09, 2023 at 01:08:32PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> When opening a directory we find what's the index of its last entry and
> then store it in the directory's file handle private data (struct
> btrfs_file_private::last_index), so that in the case new directory entries
> are added to a directory after an opendir(3) call we don't end up in an
> infinite loop (see commit 9b378f6ad48c ("btrfs: fix infinite directory
> reads")) when calling readdir(3).
>
> However once rewinddir(3) is called, POSIX states [1] that any new
> directory entries added after the previous opendir(3) call, must be
> returned by subsequent calls to readdir(3):
>
> "The rewinddir() function shall reset the position of the directory
> stream to which dirp refers to the beginning of the directory.
> It shall also cause the directory stream to refer to the current
> state of the corresponding directory, as a call to opendir() would
> have done."
>
> We currently don't refresh the last_index field of the struct
> btrfs_file_private associated to the directory, so after a rewinddir(3)
> we are not returning any new entries added after the opendir(3) call.
>
> Fix this by finding the current last index of the directory when llseek
> is called agains the directory.
>
> This can be reproduced by the following C program provided by Ian Johnson:
>
> #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);
>
> rewinddir(dir);
>
> struct dirent *entry;
> while ((entry = readdir(dir))) {
> printf("%s\n", entry->d_name);
> }
> closedir(dir);
> return 0;
> }
Missing the reference [1] here:
[1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/rewinddir.html
>
> Reported-by: Ian Johnson <ian@ianjohnson.dev>
Now also (as per the reply in the linked thread):
Tested-by: Ian Johnson <ian@ianjohnson.dev>
> Link: https://lore.kernel.org/linux-btrfs/YR1P0S.NGASEG570GJ8@ianjohnson.dev/
> Fixes: 9b378f6ad48c ("btrfs: fix infinite directory reads")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
> fs/btrfs/inode.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index df035211bdf0..006ca4cb4788 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -5820,6 +5820,19 @@ static int btrfs_opendir(struct inode *inode, struct file *file)
> return 0;
> }
>
> +static loff_t btrfs_dir_llseek(struct file *file, loff_t offset, int whence)
> +{
> + struct btrfs_file_private *private = file->private_data;
> + int ret;
> +
> + ret = btrfs_get_dir_last_index(BTRFS_I(file_inode(file)),
> + &private->last_index);
> + if (ret)
> + return ret;
> +
> + return generic_file_llseek(file, offset, whence);
> +}
> +
> struct dir_entry {
> u64 ino;
> u64 offset;
> @@ -10893,7 +10906,7 @@ static const struct inode_operations btrfs_dir_inode_operations = {
> };
>
> static const struct file_operations btrfs_dir_file_operations = {
> - .llseek = generic_file_llseek,
> + .llseek = btrfs_dir_llseek,
> .read = generic_read_dir,
> .iterate_shared = btrfs_real_readdir,
> .open = btrfs_opendir,
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] btrfs: updates for directory reading
2023-09-09 12:08 [PATCH 0/2] btrfs: updates for directory reading fdmanana
2023-09-09 12:08 ` [PATCH 1/2] btrfs: set last dir index to the current last index when opening dir fdmanana
2023-09-09 12:08 ` [PATCH 2/2] btrfs: refresh dir last index during a rewinddir(3) call fdmanana
@ 2023-09-11 17:28 ` David Sterba
2023-09-11 17:35 ` Josef Bacik
3 siblings, 0 replies; 8+ messages in thread
From: David Sterba @ 2023-09-11 17:28 UTC (permalink / raw)
To: fdmanana; +Cc: linux-btrfs, ian, Filipe Manana
On Sat, Sep 09, 2023 at 01:08:30PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> Tweak and fix a bug when reading directory entries after a rewinddir(3)
> call. Reported by Ian Johnson.
>
> Link: https://lore.kernel.org/linux-btrfs/YR1P0S.NGASEG570GJ8@ianjohnson.dev/T/#u
>
> Filipe Manana (2):
> btrfs: set last dir index to the current last index when opening dir
> btrfs: refresh dir last index during a rewinddir(3) call
Thanks for the quick fixes, added to misc-next.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] btrfs: updates for directory reading
2023-09-09 12:08 [PATCH 0/2] btrfs: updates for directory reading fdmanana
` (2 preceding siblings ...)
2023-09-11 17:28 ` [PATCH 0/2] btrfs: updates for directory reading David Sterba
@ 2023-09-11 17:35 ` Josef Bacik
2023-09-11 17:40 ` Filipe Manana
3 siblings, 1 reply; 8+ messages in thread
From: Josef Bacik @ 2023-09-11 17:35 UTC (permalink / raw)
To: fdmanana; +Cc: linux-btrfs, ian, Filipe Manana
On Sat, Sep 09, 2023 at 01:08:30PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> Tweak and fix a bug when reading directory entries after a rewinddir(3)
> call. Reported by Ian Johnson.
>
> Link: https://lore.kernel.org/linux-btrfs/YR1P0S.NGASEG570GJ8@ianjohnson.dev/T/#u
>
> Filipe Manana (2):
> btrfs: set last dir index to the current last index when opening dir
> btrfs: refresh dir last index during a rewinddir(3) call
>
> fs/btrfs/inode.c | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
Reviewed-by: Josef Bacik <josef@toxicpanda.com>
I didn't see an fstest for this, is it forthcoming? Thanks,
Josef
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 0/2] btrfs: updates for directory reading
2023-09-11 17:35 ` Josef Bacik
@ 2023-09-11 17:40 ` Filipe Manana
0 siblings, 0 replies; 8+ messages in thread
From: Filipe Manana @ 2023-09-11 17:40 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-btrfs, ian, Filipe Manana
On Mon, Sep 11, 2023 at 6:35 PM Josef Bacik <josef@toxicpanda.com> wrote:
>
> On Sat, Sep 09, 2023 at 01:08:30PM +0100, fdmanana@kernel.org wrote:
> > From: Filipe Manana <fdmanana@suse.com>
> >
> > Tweak and fix a bug when reading directory entries after a rewinddir(3)
> > call. Reported by Ian Johnson.
> >
> > Link: https://lore.kernel.org/linux-btrfs/YR1P0S.NGASEG570GJ8@ianjohnson.dev/T/#u
> >
> > Filipe Manana (2):
> > btrfs: set last dir index to the current last index when opening dir
> > btrfs: refresh dir last index during a rewinddir(3) call
> >
> > fs/btrfs/inode.c | 18 ++++++++++++++++--
> > 1 file changed, 16 insertions(+), 2 deletions(-)
> >
>
> Reviewed-by: Josef Bacik <josef@toxicpanda.com>
>
> I didn't see an fstest for this, is it forthcoming? Thanks,
Give me a break... I did this on a Saturday morning.
Be patient...
>
> Josef
^ permalink raw reply [flat|nested] 8+ messages in thread