[PATCH, RFC] ext4: remove ext4_dir_llseek

[PATCH, RFC] ext4: remove ext4_dir_llseek

[Eric Sandeen]

ext4_dir_llseek() was recently added as part of a patch to allow
returning 64-bit name hashes.  However, reworking _llseek() is not
necessary to achieve that goal.  One unfortunate side effect of the
change is that it cut&pasted VFS code back into ext4, after Andi
had just removed other _llseek cut&paste in ext4 by extending the
VFS functionality.

It also re-introduced i_mutex locking in the dir llseek paths,
un-doing the upstream (mostly) lockless llseek changes from Andi
in this case.

Because of the above reasons, and because it introduces new
EINVAL returns which were not there before, and because SEEK_END+offset
behaves differently from SEEK_SET w.r.t. offset limits, and because
it's not clear what problem this is solving, remove it for now.

(NFS only uses SEEK_SET and SEEK_CUR, so changes to SEEK_END shouldn't
affect it.)

If & when a problematic and testable real-world use case is described,
this can be re-fixed properly, possibly by expanding the VFS _llseek()
functions to handle custom EOF offsets for cases such as this.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

Ok, I'm being something of a devil's advocate here, but - if the change
is to stay, it needs to be documented & justified with a real-world
usecase, I think?  The net effect of the changes seems to be using the
max hash value for offset limits and for EOF, not i_size and s_maxbytes.

<viro> frankly, seeks on directories have always been a source of PITA
<viro> what userland code do you have that does SEEK_END - offset on directories?

and - I didn't have an answer...

I do also have a VFS patch in my back pocket to override EOF & s_maxbytes limits,
but we'll need a good explanation of why it's warranted, I think.

diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
index b867862..4542d30 100644
--- a/fs/ext4/dir.c
+++ b/fs/ext4/dir.c
@@ -310,78 +310,6 @@ static inline loff_t ext4_get_htree_eof(struct file *filp)
 		return EXT4_HTREE_EOF_64BIT;
 }
 
-
-/*
- * ext4_dir_llseek() based on generic_file_llseek() to handle both
- * non-htree and htree directories, where the "offset" is in terms
- * of the filename hash value instead of the byte offset.
- *
- * NOTE: offsets obtained *before* ext4_set_inode_flag(dir, EXT4_INODE_INDEX)
- *       will be invalid once the directory was converted into a dx directory
- */
-loff_t ext4_dir_llseek(struct file *file, loff_t offset, int origin)
-{
-	struct inode *inode = file->f_mapping->host;
-	loff_t ret = -EINVAL;
-	int dx_dir = is_dx_dir(inode);
-
-	mutex_lock(&inode->i_mutex);
-
-	/* NOTE: relative offsets with dx directories might not work
-	 *       as expected, as it is difficult to figure out the
-	 *       correct offset between dx hashes */
-
-	switch (origin) {
-	case SEEK_END:
-		if (unlikely(offset > 0))
-			goto out_err; /* not supported for directories */
-
-		/* so only negative offsets are left, does that have a
-		 * meaning for directories at all? */
-		if (dx_dir)
-			offset += ext4_get_htree_eof(file);
-		else
-			offset += inode->i_size;
-		break;
-	case SEEK_CUR:
-		/*
-		 * Here we special-case the lseek(fd, 0, SEEK_CUR)
-		 * position-querying operation.  Avoid rewriting the "same"
-		 * f_pos value back to the file because a concurrent read(),
-		 * write() or lseek() might have altered it
-		 */
-		if (offset == 0) {
-			offset = file->f_pos;
-			goto out_ok;
-		}
-
-		offset += file->f_pos;
-		break;
-	}
-
-	if (unlikely(offset < 0))
-		goto out_err;
-
-	if (!dx_dir) {
-		if (offset > inode->i_sb->s_maxbytes)
-			goto out_err;
-	} else if (offset > ext4_get_htree_eof(file))
-		goto out_err;
-
-	/* Special lock needed here? */
-	if (offset != file->f_pos) {
-		file->f_pos = offset;
-		file->f_version = 0;
-	}
-
-out_ok:
-	ret = offset;
-out_err:
-	mutex_unlock(&inode->i_mutex);
-
-	return ret;
-}

Re: [PATCH, RFC] ext4: remove ext4_dir_llseek

[Eric Sandeen]

On 4/25/12 2:23 PM, Eric Sandeen wrote:
> ext4_dir_llseek() was recently added as part of a patch to allow
> returning 64-bit name hashes.  However, reworking _llseek() is not
> necessary to achieve that goal.  One unfortunate side effect of the
> change is that it cut&pasted VFS code back into ext4, after Andi
> had just removed other _llseek cut&paste in ext4 by extending the
> VFS functionality.
> 
> It also re-introduced i_mutex locking in the dir llseek paths,
> un-doing the upstream (mostly) lockless llseek changes from Andi
> in this case.
> 
> Because of the above reasons, and because it introduces new
> EINVAL returns which were not there before, and because SEEK_END+offset
> behaves differently from SEEK_SET w.r.t. offset limits, and because
> it's not clear what problem this is solving, remove it for now.
> 
> (NFS only uses SEEK_SET and SEEK_CUR, so changes to SEEK_END shouldn't
> affect it.)
> 
> If & when a problematic and testable real-world use case is described,
> this can be re-fixed properly, possibly by expanding the VFS _llseek()
> functions to handle custom EOF offsets for cases such as this.

Self-NAK.  Realized this won't work for non-extent dirs, because the
generic llseek will EINVAL for hash "offsets" larger than the max_bitmap_bytes
value.  Will send something a little less drastic.

-Eric

[PATCH] ext4: call vfs llseek code from ext4_dir_llseek

[Eric Sandeen]

ext4_dir_llseek can call the vfs llseek functions and get all the
the functionality needed by the NFS server.

For htree directories, we call the _sized() variant with the
maximum allowable hash value.  For non-htree, we can call
ext4_file_llseek, which will sort out the maximum allowable
offset based on whether the dir is extent based or not.

This does lose the special SEEK_END handling of going to the
max hash size, but I know of no usecase for that and no bugs,
despite SEEK_END going to i_size and not the hash EOF for
many years now, so I don't think the cut & paste is warranted.
The vfs function can be expanded if it's critical.

This also returns dir llseek to it's (mostly) lockless
implementation.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
index b867862..faebc89 100644
--- a/fs/ext4/dir.c
+++ b/fs/ext4/dir.c
@@ -312,74 +312,26 @@ static inline loff_t ext4_get_htree_eof(struct file *filp)
 
 
 /*
- * ext4_dir_llseek() based on generic_file_llseek() to handle both
- * non-htree and htree directories, where the "offset" is in terms
- * of the filename hash value instead of the byte offset.
+ * ext4_dir_llseek() calls generic_file_llseek_size to handle htree
+ * directories, where the "offset" is in terms of the filename hash
+ * value instead of the byte offset.
  *
- * NOTE: offsets obtained *before* ext4_set_inode_flag(dir, EXT4_INODE_INDEX)
- *       will be invalid once the directory was converted into a dx directory
+ * Because we may return a 64-bit hash that is well beyond offset limits,
+ * we need to pass the max hash as the maximum allowable offset in
+ * the htree directory case.
+ *
+ * For non-htree, ext4_llseek already chooses the proper max offset.
  */
 loff_t ext4_dir_llseek(struct file *file, loff_t offset, int origin)
 {
 	struct inode *inode = file->f_mapping->host;
-	loff_t ret = -EINVAL;
 	int dx_dir = is_dx_dir(inode);
 
-	mutex_lock(&inode->i_mutex);
-
-	/* NOTE: relative offsets with dx directories might not work
-	 *       as expected, as it is difficult to figure out the
-	 *       correct offset between dx hashes */
-
-	switch (origin) {
-	case SEEK_END:
-		if (unlikely(offset > 0))
-			goto out_err; /* not supported for directories */
-
-		/* so only negative offsets are left, does that have a
-		 * meaning for directories at all? */
-		if (dx_dir)
-			offset += ext4_get_htree_eof(file);
-		else
-			offset += inode->i_size;
-		break;
-	case SEEK_CUR:
-		/*
-		 * Here we special-case the lseek(fd, 0, SEEK_CUR)
-		 * position-querying operation.  Avoid rewriting the "same"
-		 * f_pos value back to the file because a concurrent read(),
-		 * write() or lseek() might have altered it
-		 */
-		if (offset == 0) {
-			offset = file->f_pos;
-			goto out_ok;
-		}
-
-		offset += file->f_pos;
-		break;
-	}
-
-	if (unlikely(offset < 0))
-		goto out_err;
-
-	if (!dx_dir) {
-		if (offset > inode->i_sb->s_maxbytes)
-			goto out_err;
-	} else if (offset > ext4_get_htree_eof(file))
-		goto out_err;
-
-	/* Special lock needed here? */
-	if (offset != file->f_pos) {
-		file->f_pos = offset;
-		file->f_version = 0;
-	}
-
-out_ok:
-	ret = offset;
-out_err:
-	mutex_unlock(&inode->i_mutex);