On Tue, Sep 3, 2019 at 11:31 PM Deepa Dinamani <deepa.ker...@gmail.com> wrote:
> On Tue, Sep 3, 2019 at 2:18 PM Theodore Y. Ts'o <ty...@mit.edu> wrote:
> > On Tue, Sep 03, 2019 at 09:18:44AM -0700, Deepa Dinamani wrote:
> > >
> > > This prints a warning for each inode that doesn't extend limits beyond
> > > 2038. It is rate limited by the ext4_warning_inode().
> > > Looks like your filesystem has inodes that cannot be extended.
> > > We could use a different rate limit or ignore this corner case. Do the
> > > maintainers have a preference?
> >
> > We need to drop this commit (ext4: Initialize timestamps limits), or
> > at least the portion which adds the call to the EXT4_INODE_SET_XTIME
> > macro in ext4.h.
>
> As Arnd said, I think this can be fixed by warning only when the inode
> size is not uniformly 128 bytes in ext4.h. Is this an acceptable
> solution or we want to drop this warning altogether?
I think the warning as it was intended makes sense, the idea
was never to warn on every inode update for file systems that
cannot handle future dates, only to warn when we
a) try to set a future date
b) fail to do that because the space cannot be made available.
> Arnd, should I be sending a pull request again with the fix? Or, we
> drop the ext4 patch and I can send it to the maintainers directly?
I would prefer to fix it on top of the patches I already merged.
Maybe something like:
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 9e3ae3be3de9..5a971d1b6d5e 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -835,7 +835,9 @@ do {
\
}
\
else {\
(raw_inode)->xtime = cpu_to_le32(clamp_t(int32_t,
(inode)->xtime.tv_sec, S32_MIN, S32_MAX)); \
- ext4_warning_inode(inode, "inode does not support
timestamps beyond 2038"); \
+ if (((inode)->xtime.tv_sec != (raw_inode)->xtime) && \
+ ((inode)->i_sb->s_time_max > S32_MAX))
\
+ ext4_warning_inode(inode, "inode does not
support timestamps beyond 2038"); \
} \
} while (0)
> > In cases where the inode size is such that there is no chance at all
> > to support timestamps beyond 2038, a single warning at mount time, or
> > maybe a warning at mkfs time might be acceptable. But there's no
> > point printing a warning time each time we set a timestamp on such a
> > file system. It's not going to change, and past a certain point, we
> > need to trust that people who are using 128 byte inodes did so knowing
> > what the tradeoffs might be. After all, it is *not* the default.
>
> We have a single mount time warning already in place here. I did not
> realize some people actually chose to use 128 byte inodes on purpose.
This is also new to me, as I always assumed a normal ext4 would be y2038
safe. I suspect that a few of those users are unaware of the y2038
problem they might run into because of that, but that's what the mount-time
warning should help with.
However, I did expect that people might have legacy ext3 file system
images that they mount, and printing a warning for each write would
also be wrong for those.
Arnd
