On Tue, Jun 04, 2013 at 04:26:57PM -0700, Zach Brown wrote:
> On Tue, Jun 04, 2013 at 07:16:53PM -0400, Chris Mason wrote:
> > Quoting Zach Brown (2013-06-04 18:17:54)
> > > Hi gang,
> > >
> > > I finally sat down to fix that readdir hang that has been in the back
> > > of my mind for a while. I *hope* that the fix is pretty simple: just
> > > don't manufacture a fake f_pos, I *think* we can abuse f_version as an
> > > indicator that we shouldn't return entries. Does this look reasonable?
> >
> > I like it, and it doesn't look too far away from how others are abusing
> > f_version. Have you tried with NFS? I don't think it'll hurt, but NFS
> > loves to surprise me.
>
> Mm, no, I hadn't. I'll give it a go tomorrow. What could go wrong? :)
Or a week later. Pretty close!
I couldn't get NFS to break. Clients see new entries created directly
in the exported btrfs and on either of noac and actime=1 client mounts.
For whatever that's worth.
But I did find that I'd broken the case of trying to re-enable readdir
results by seeking past the last entry (which happens to be the current
f_pos now that we're using f_version).
Here's the incremental fix against what Josef has in -next. I'm cool
with either squashing or just committing it.
- z
Subject: [PATCH] btrfs: reset f_version when seeking to pos
Commit 63e3dfe ("btrfs: fix readdir hang with offsets past INT_MAX")
switched to using f_version to stop readdir results instead of setting a
large f_pos.
It inadvertantly changed behaviour in the case where an app specifically
seeks to one past the last valid dent->d_off it has seen. Previously
f_pos would have changed from the fake f_pos to this new f_pos which
would let readdir return new entries.
But now that it's using f_version it might not have seen new entries.
generic_file_llseek() won't clear f_version if the desirned pos happens
to be the current f_pos.
So we add a little wrapper to notice this case and clear f_version so
that entries can be seen in this case.
Signed-off-by: Zach Brown <zab@xxxxxxxxxx>
---
fs/btrfs/inode.c | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 1059c90..590c274 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4997,6 +4997,23 @@ unsigned char btrfs_filetype_table[] = {
* which prevents readdir results until seek resets f_pos and f_version.
*/
#define BTRFS_READDIR_EOF ~0ULL
+static loff_t btrfs_dir_llseek(struct file *file, loff_t offset, int whence)
+{
+ struct inode *inode = file->f_mapping->host;
+ loff_t ret;
+
+ /*
+ * f_version isn't reset if a seek is attempted to the current pos. A
+ * caller can be trying to see more entries by seeking past the last
+ * entry to the current pos after creating a new entry.
+ */
+ mutex_lock(&inode->i_mutex);
+ ret = generic_file_llseek(file, offset, whence);
+ if (ret == offset && file->f_version == BTRFS_READDIR_EOF)
+ file->f_version = 0;
+ mutex_unlock(&inode->i_mutex);
+ return ret;
+}
static int btrfs_real_readdir(struct file *filp, void *dirent,
filldir_t filldir)
@@ -8642,7 +8659,7 @@ static const struct inode_operations btrfs_dir_ro_inode_operations = {
};
static const struct file_operations btrfs_dir_file_operations = {
- .llseek = generic_file_llseek,
+ .llseek = btrfs_dir_llseek,
.read = generic_read_dir,
.readdir = btrfs_real_readdir,
.unlocked_ioctl = btrfs_ioctl,
--
1.7.11.7
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
