ext4: remove ext4_dir_llseek
RFC PATCH 1/1 ext4: remove ext4_dir_llseek
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.
---
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.
fs/ext4/dir.c +0 -71
@@ -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; -}
Quoted 16 lines · 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_llseekcut&paste in ext4 by extending the VFS functionality.It also re-introduced
i_mutexlocking 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_SETw.r.t. offset limits, and because it's not clear what problem this is solving, remove it for now.
Quoted 6 lines
(NFS only uses
SEEK_SETandSEEK_CUR, so changes toSEEK_ENDshouldn'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 1/1 ext4: call vfs llseek code from ext4_dir_llseek
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.
---
fs/ext4/dir.c +8 -59
@@ -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);