Bug#738758: [PATCH] ext4: kill i_version support for Hurd-castrated file systems
On Thu, Mar 20, 2014 at 05:30:32PM +, Ben Hutchings wrote: > > It looks like ext2 and ext3 would always initialise i_version to 1 in > memory; does it matter that you're changing that to 0 for Hurd > filesystems? No, NFS only cares that the i_version number has changed, and it's mainly important if you have two clients trying to simultaneously access the same file, so they get a signal that they need to invalidate their locally cached metadata (or data, in the case of NFSv4). But Hurd only supports NFSv2, and the performance is such that I doubt anyone would be all that interested in using a Hurd server as a NFS server for even a small workgroup, let alone a department, so this is unlikely to be a big deal. Cheers, - Ted -- To UNSUBSCRIBE, email to debian-kernel-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org Archive: https://lists.debian.org/20140321143927.ga31...@thunk.org
Bug#738758: [PATCH] ext4: kill i_version support for Hurd-castrated file systems
On Thu, 2014-03-20 at 10:44 -0400, ty...@mit.edu wrote: > On Thu, Mar 20, 2014 at 01:10:48AM -0700, Christoph Hellwig wrote: > > On Wed, Mar 19, 2014 at 11:27:57PM -0600, Andreas Dilger wrote: > > > Probably worthwhile to make those !EXT4_OS_HURD checks likely()? > > Yes, and I was planning on optimizing the checks a bit more, but it > makes sense to do that in a separate patch, since this is not the only > place where we are making EXT4_OS_HURD checks. > > > > > Does it make sense to support the format at all given that it's unlikely > > to get any testing? > > There are some crazy people still trying to make the Hurd a viable > file system. There's even a Debian port for it. :-) The problem is > that some of the folks who are still trying to make the Hurd real want > to use ext2 as an interchange format between Linux and Hurd, and > presumably that's how they ran across this particular bug. [...] That, plus we turned on CONFIG_EXT4_USE_FOR_EXT23 for Debian kernel packages starting with Linux 3.11. It looks like ext2 and ext3 would always initialise i_version to 1 in memory; does it matter that you're changing that to 0 for Hurd filesystems? Ben. -- Ben Hutchings One of the nice things about standards is that there are so many of them. signature.asc Description: This is a digitally signed message part
Bug#738758: [PATCH] ext4: kill i_version support for Hurd-castrated file systems
On Thu, Mar 20, 2014 at 01:10:48AM -0700, Christoph Hellwig wrote: > On Wed, Mar 19, 2014 at 11:27:57PM -0600, Andreas Dilger wrote: > > Probably worthwhile to make those !EXT4_OS_HURD checks likely()? Yes, and I was planning on optimizing the checks a bit more, but it makes sense to do that in a separate patch, since this is not the only place where we are making EXT4_OS_HURD checks. > > Does it make sense to support the format at all given that it's unlikely > to get any testing? There are some crazy people still trying to make the Hurd a viable file system. There's even a Debian port for it. :-) The problem is that some of the folks who are still trying to make the Hurd real want to use ext2 as an interchange format between Linux and Hurd, and presumably that's how they ran across this particular bug. While I think Hurd is a joke, and it's laughable that the primary file system for the Hurd doesn't support journalling, or extents, delayed allocation, or extended attributes[1], and it's hard for them get feature parity because of the GPL v3 vs v2 compatibility problem, I don't mind making minimal efforts to provide legacy support to the GNU Hurd. [1] http://savannah.gnu.org/patch/?5126 (Patches available for the last seven years, not yet integrated) In the long run, the right answer is that the Hurd should use an extended attribute to store the translator information. Indeed, this has been discussed seven years ago[2]. But unfortunately, the development temp for Hurd is a wee bit slower than for the Linux kernel. :-) [2] http://savannah.gnu.org/task/?5503 - Ted -- To UNSUBSCRIBE, email to debian-kernel-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org Archive: https://lists.debian.org/20140320144456.ga20...@thunk.org
Bug#738758: [PATCH] ext4: kill i_version support for Hurd-castrated file systems
On Wed, Mar 19, 2014 at 11:27:57PM -0600, Andreas Dilger wrote: > Probably worthwhile to make those !EXT4_OS_HURD checks likely()? Does it make sense to support the format at all given that it's unlikely to get any testing? -- To UNSUBSCRIBE, email to debian-kernel-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org Archive: https://lists.debian.org/20140320081048.ga21...@infradead.org
Bug#738758: [PATCH] ext4: kill i_version support for Hurd-castrated file systems
Probably worthwhile to make those !EXT4_OS_HURD checks likely()?
Cheers, Andreas
> On Mar 19, 2014, at 22:34, Theodore Ts'o wrote:
>
> The Hurd file system uses uses the inode field which is now used for
> i_version for its translator block. This means that ext2 file systems
> that are formatted for GNU Hurd can't be used to support NFSv4. Given
> that Hurd file systems don't support extents, and a huge number of
> modern file system features, this is no great loss.
>
> If we don't do this, the attempt to update the i_version field will
> stomp over the translator block field, which will cause file system
> corruption for Hurd file systems. This can be replicated via:
>
> mke2fs -t ext2 -o hurd /dev/vdc
> mount -t ext4 /dev/vdc /vdc
> touch /vdc/bug
> umount /dev/vdc
> e2fsck -f /dev/vdc
>
> Addresses-Debian-Bug: #738758
>
> Reported-By: Gabriele Giacone <1o5g4...@gmail.com>
> Signed-off-by: "Theodore Ts'o"
> ---
> fs/ext4/inode.c | 29 ++---
> 1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 7cc2455..ed2c13a 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4168,11 +4168,14 @@ struct inode *ext4_iget(struct super_block *sb,
> unsigned long ino)
>EXT4_INODE_GET_XTIME(i_atime, inode, raw_inode);
>EXT4_EINODE_GET_XTIME(i_crtime, ei, raw_inode);
>
> -inode->i_version = le32_to_cpu(raw_inode->i_disk_version);
> -if (EXT4_INODE_SIZE(inode->i_sb) > EXT4_GOOD_OLD_INODE_SIZE) {
> -if (EXT4_FITS_IN_INODE(raw_inode, ei, i_version_hi))
> -inode->i_version |=
> -(__u64)(le32_to_cpu(raw_inode->i_version_hi)) << 32;
> +if (EXT4_SB(inode->i_sb)->s_es->s_creator_os !=
> +cpu_to_le32(EXT4_OS_HURD)) {
> +inode->i_version = le32_to_cpu(raw_inode->i_disk_version);
> +if (EXT4_INODE_SIZE(inode->i_sb) > EXT4_GOOD_OLD_INODE_SIZE) {
> +if (EXT4_FITS_IN_INODE(raw_inode, ei, i_version_hi))
> +inode->i_version |=
> +(__u64)(le32_to_cpu(raw_inode->i_version_hi)) << 32;
> +}
>}
>
>ret = 0;
> @@ -4388,12 +4391,16 @@ static int ext4_do_update_inode(handle_t *handle,
>raw_inode->i_block[block] = ei->i_data[block];
>}
>
> -raw_inode->i_disk_version = cpu_to_le32(inode->i_version);
> -if (ei->i_extra_isize) {
> -if (EXT4_FITS_IN_INODE(raw_inode, ei, i_version_hi))
> -raw_inode->i_version_hi =
> -cpu_to_le32(inode->i_version >> 32);
> -raw_inode->i_extra_isize = cpu_to_le16(ei->i_extra_isize);
> +if (EXT4_SB(inode->i_sb)->s_es->s_creator_os !=
> +cpu_to_le32(EXT4_OS_HURD)) {
> +raw_inode->i_disk_version = cpu_to_le32(inode->i_version);
> +if (ei->i_extra_isize) {
> +if (EXT4_FITS_IN_INODE(raw_inode, ei, i_version_hi))
> +raw_inode->i_version_hi =
> +cpu_to_le32(inode->i_version >> 32);
> +raw_inode->i_extra_isize =
> +cpu_to_le16(ei->i_extra_isize);
> +}
>}
>
>ext4_inode_csum_set(inode, raw_inode, ei);
> --
> 1.9.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To UNSUBSCRIBE, email to debian-kernel-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Archive: https://lists.debian.org/3df297f8-04e0-4ee6-8cd5-127997293...@dilger.ca
Bug#738758: [PATCH] ext4: kill i_version support for Hurd-castrated file systems
The Hurd file system uses uses the inode field which is now used for
i_version for its translator block. This means that ext2 file systems
that are formatted for GNU Hurd can't be used to support NFSv4. Given
that Hurd file systems don't support extents, and a huge number of
modern file system features, this is no great loss.
If we don't do this, the attempt to update the i_version field will
stomp over the translator block field, which will cause file system
corruption for Hurd file systems. This can be replicated via:
mke2fs -t ext2 -o hurd /dev/vdc
mount -t ext4 /dev/vdc /vdc
touch /vdc/bug
umount /dev/vdc
e2fsck -f /dev/vdc
Addresses-Debian-Bug: #738758
Reported-By: Gabriele Giacone <1o5g4...@gmail.com>
Signed-off-by: "Theodore Ts'o"
---
fs/ext4/inode.c | 29 ++---
1 file changed, 18 insertions(+), 11 deletions(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 7cc2455..ed2c13a 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4168,11 +4168,14 @@ struct inode *ext4_iget(struct super_block *sb,
unsigned long ino)
EXT4_INODE_GET_XTIME(i_atime, inode, raw_inode);
EXT4_EINODE_GET_XTIME(i_crtime, ei, raw_inode);
- inode->i_version = le32_to_cpu(raw_inode->i_disk_version);
- if (EXT4_INODE_SIZE(inode->i_sb) > EXT4_GOOD_OLD_INODE_SIZE) {
- if (EXT4_FITS_IN_INODE(raw_inode, ei, i_version_hi))
- inode->i_version |=
- (__u64)(le32_to_cpu(raw_inode->i_version_hi)) << 32;
+ if (EXT4_SB(inode->i_sb)->s_es->s_creator_os !=
+ cpu_to_le32(EXT4_OS_HURD)) {
+ inode->i_version = le32_to_cpu(raw_inode->i_disk_version);
+ if (EXT4_INODE_SIZE(inode->i_sb) > EXT4_GOOD_OLD_INODE_SIZE) {
+ if (EXT4_FITS_IN_INODE(raw_inode, ei, i_version_hi))
+ inode->i_version |=
+ (__u64)(le32_to_cpu(raw_inode->i_version_hi)) << 32;
+ }
}
ret = 0;
@@ -4388,12 +4391,16 @@ static int ext4_do_update_inode(handle_t *handle,
raw_inode->i_block[block] = ei->i_data[block];
}
- raw_inode->i_disk_version = cpu_to_le32(inode->i_version);
- if (ei->i_extra_isize) {
- if (EXT4_FITS_IN_INODE(raw_inode, ei, i_version_hi))
- raw_inode->i_version_hi =
- cpu_to_le32(inode->i_version >> 32);
- raw_inode->i_extra_isize = cpu_to_le16(ei->i_extra_isize);
+ if (EXT4_SB(inode->i_sb)->s_es->s_creator_os !=
+ cpu_to_le32(EXT4_OS_HURD)) {
+ raw_inode->i_disk_version = cpu_to_le32(inode->i_version);
+ if (ei->i_extra_isize) {
+ if (EXT4_FITS_IN_INODE(raw_inode, ei, i_version_hi))
+ raw_inode->i_version_hi =
+ cpu_to_le32(inode->i_version >> 32);
+ raw_inode->i_extra_isize =
+ cpu_to_le16(ei->i_extra_isize);
+ }
}
ext4_inode_csum_set(inode, raw_inode, ei);
--
1.9.0
--
To UNSUBSCRIBE, email to debian-kernel-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Archive:
https://lists.debian.org/1395290051-25682-1-git-send-email-ty...@mit.edu
