Re: [Y2038] [PATCH v6 3/4] vfs: Add timestamp_truncate() api
On Wed, Jan 24, 2018 at 3:56 AM, Arnd Bergmann wrote: > On Tue, Jan 23, 2018 at 5:25 PM, Deepa Dinamani > wrote: >> >> I checked POSIX again. There is no mention of tv_nsec being positive >> always for utimes. >> And, the long term plan is to replace all the callers of >> timespec_trunc() to use this new api instead for filesystems. >> So this will need to be fixed. I will fix this and post an update. > > I found this on > http://pubs.opengroup.org/onlinepubs/9699919799/functions/utimes.html > > ERRORS > These functions shall fail if: > ... > [EINVAL] > Either of the times argument structures specified a tv_nsec value that was > neither UTIME_NOW nor UTIME_OMIT, and was a value less than zero or > greater than or equal to 1000 million. The thing is, we shouldn't check the standards, we should check what we actually _do_. And what we actually _do_ is to have a tv_nsec that is of type "long", and while we do have a timespec64_valid(const struct timespec64 *ts) and fs/utimes.c has a 'nsec_valid()' that apparently the utimes* family of system calls all do end up using, I'm more worried about something where we don't. Because I'm almost certain that we've had cases where we just normalize things after-the-fact. This all likely isn't a big deal, but it does end up worrying me if we then _depend_ on that granularity thing actually giving the proper granularity even for odd inputs. If they can happen. Maybe we don't care? Linus ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH v6 3/4] vfs: Add timestamp_truncate() api
On Sun, Jan 21, 2018 at 6:04 PM, Deepa Dinamani wrote: > + t.tv_nsec -= t.tv_nsec % gran; This doesn't actuall ywork if tv_nsec is negative. Which may not be an issue in most cases, but did somebody check utimensat() or whatever? > + WARN(1, "illegal file time granularity: %u", gran); .. small nit: we generally should use 'invalid' rather than 'illegal'. No cops will hunt you down for things like this. Linus ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH v5 2/5] vfs: Add checks for filesystem timestamp limits
On Tue, Apr 25, 2017 at 1:31 PM, Arnd Bergmann wrote: > > Would it be ok to have a simple way of removing the time_t definition (e.g. > by passing '-DREQUIRE_TIME64' to the compiler, but without the Kconfig > option? That way, someone who wants to ship a product can at least > find the obvious dependencies on stuff that remains broken. How would you find them? People don't necessarily use "time_t". They might use "int" or whatever. There is absolutely zero point to making this some kind of crazy config option, because such an option will prove absolutely *NOTHING*. Seriously. This whole concept is completely stupid. The only possible thing you can do is to (a) have an actual test-suite (b) set the time to 32+ bits (c) see what breaks because otherwise it seems entirely pointless. And no, we're not adding random crazy source modifications for pointless crap. Linus ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH v5 2/5] vfs: Add checks for filesystem timestamp limits
On Tue, Apr 25, 2017 at 12:47 PM, Arnd Bergmann wrote: > > There is one global option that I want to see, and that is for completely > disabling all components that are known to be broken in y2038. I really don't see the point. Don't do it. Make it some local hack, I'm not taking crazy patches. Linus ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH v5 2/5] vfs: Add checks for filesystem timestamp limits
On Sat, Apr 8, 2017 at 12:37 PM, Deepa Dinamani wrote: > Allow read only mounts for filesystems that do not > have maximum timestamps beyond the y2038 expiry > timestamp. This option seems arbitrary and pointless. Nobody sane should ever enable it except for testing, but for testing it would be much better to simply specify what the limit should be: 2038 is not magical for all filesystems, because the base may be different. And honestly, for testing, it would be much better to just make it a mount option rather than some crazy system-wide one. Linus ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH v5 0/5] Introduce current_time() api
On Wed, Sep 14, 2016 at 7:48 AM, Deepa Dinamani wrote: > The series is aimed at getting rid of CURRENT_TIME and CURRENT_TIME_SEC > macros. > The macros are not y2038 safe. There is no plan to transition them into being > y2038 safe. This version looks ok to me. Al, would you be willing to queue this up for 4.9? Linus ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH v2 00/24] Delete CURRENT_TIME and CURRENT_TIME_SEC macros
On Sun, Jun 19, 2016 at 5:26 PM, Deepa Dinamani wrote: > The series is aimed at getting rid of CURRENT_TIME and CURRENT_TIME_SEC > macros. This version now looks ok to me. I do have a comment (or maybe just a RFD) for future work. It does strike me that once we actually change over the inode times to use timespec64, the calling conventions are going to be fairly horrendous on most 32-bit architectures. Gcc handles 8-byte structure returns (on most architectures) by returning them as two 32-bit registers (%edx:%eax on x86). But once it is timespec64, that will no longer be the case, and the calling convention will end up using a pointer to the local stack instead. So for 32-bit code generation, we *may* want to introduce a new model of doing set_inode_time(inode, ATTR_ATIME | ATTR_MTIME); which basically just does inode->i_atime = inode->i_mtime = current_time(inode); but with a much easier calling convention on 32-bit architectures. But that is entirely orthogonal to this patch-set, and should be seen as a separate issue. And maybe it doesn't end up helping anyway, but I do think those "simple" direct assignments will really generate pretty disgusting code on 32-bit architectures. That whole inode->i_atime = inode->i_mtime = CURRENT_TIME; model really made a lot more sense back in the ancient days when inode times were just simply 32-bit seconds (not even timeval structures). Linus ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH 04/21] fs: Replace CURRENT_TIME with current_fs_time() for inode timestamps
On Thu, Jun 9, 2016 at 1:38 PM, Deepa Dinamani wrote: > > 1. There are a few link, rename functions which assign times like this: > > - inode->i_ctime = dir->i_ctime = dir->i_mtime = CURRENT_TIME; > + inode->i_ctime = dir->i_ctime = dir->i_mtime = > current_fs_time(dir->i_sb); So I think you should just pass one any of the two inodes and just add a comment. Then, if we hit a filesystem that actually wants to have different granularity for different inodes, we'll split it up, but even then we'd be better off than with the superblock, since then we *could* easily split this one case up into "get directory time" and "get inode time". > 2. Also, this means that we will make it an absolute policy that any > filesystem > timestamp that is not directly connected to an inode would have to use > ktime_get_* apis. The thing is, those kinds of things are all going to be inside the filesystem itself. At that point, the *filesystem* already knows what the timekeeping rules for that filesystem is. I think we should strive to design the "current_fs_time()" not for internal filesystem use, but for actual generic use where we *don't* know a priori what the rules are, and we have to go to this helper function to figure it out. Inside a filesystem, why *shouldn't* the low-level filesystem already use the normal "get time" functions? See what I'm saying? The primary value-add to "current_fs_time()" is for layers like the VFS and security layer that don't know what the filesystem itself does. At the low-level filesystem layer, you may just know that "ok, I only have 32-bit timestamps anyway, so I should just use a 32-bit time function". > 3. Even if the filesystem inode has extra timestamps and these are not > part of vfs inode, we still use > vfs inode to get the timestamps from current_fs_time(): Eg: ext4 create time But those already have an inode. In fact, ext4 is a particularly bad example, since it uses the ext4_current_time() function to get the time. And that one gets an inode pointer. So at least one filesystem that already does this, already uses a inode-based model. Everything I see just says "times are about inodes". Anything else almost has to be filesystem-internal anyway, since the only thing that is ever visible outside the filesystem (time-wise) is the inode. And as mentioned, once it's internal to the low-level filesystem, it's not obvious at all that you'd have to use "currenf_fs_time()" anyway. The internal filesystem code might very well decide to use other timekeeping functions. Linus ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH 04/21] fs: Replace CURRENT_TIME with current_fs_time() for inode timestamps
On Wed, Jun 8, 2016 at 10:04 PM, Deepa Dinamani wrote:
> CURRENT_TIME macro is not appropriate for filesystems as it
> doesn't use the right granularity for filesystem timestamps.
> Use current_fs_time() instead.
Again - using the inode instead fo the syuperblock in tghis patch
would have made the patch much more obvious (it could have been 99%
generated with the sed-script I sent out a week or two ago), and it
would have made it unnecessary to add these kinds of things:
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index e9f5043..85c12f0 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -2359,6 +2359,7 @@ static long usbdev_do_ioctl(struct file *file, unsigned
> int cmd,
> {
> struct usb_dev_state *ps = file->private_data;
> struct inode *inode = file_inode(file);
> + struct super_block *sb = inode->i_sb;
> struct usb_device *dev = ps->dev;
> int ret = -ENOTTY;
where we add a new variable just because the calling convention was wrong.
It's not even 100% obvious that a filesystem has to have one single
time representation, so making the time function about the entity
whose time is set is also conceptually a much better model, never mind
that it is just what every single user seems to want anyway.
So I'd *much* rather see
+ inode->i_atime = inode->i_mtime = inode->i_ctime =
current_fs_time(inode);
over seeing either of these two variants::
+ inode->i_atime = inode->i_mtime = inode->i_ctime =
current_fs_time(inode->i_sb);
+ ret->i_atime = ret->i_mtime = ret->i_ctime = current_fs_time(sb);
because the first of those variants (grep for current_fs_time() in the
current git tree, and notice that it's the common one) we have the
pointless "let's chase a pointer in every caller"
And while it's true that the second variant is natural for *some*
situations, I've yet to find one where it wasn't equally sane to just
pass in the inode instead.
Linus
___
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH 01/21] fs: Replace CURRENT_TIME_SEC with current_fs_time()
On Thu, Jun 9, 2016 at 12:35 AM, Jan Kara wrote: > > You create line longer than 80 characters for affs and reiserfs. Please > wrap those lines properly. No, please do *NOT* do things like that. These kind of mechanical patches should (a) be as mechanical as possible (and see elsewhere about why I think 'sb' should be 'inode' and the patch should have been 95% automated with a trivial script thanks to that change) (b) be made as easy to verify visually as possible. That (b) means that a conversion should *not* add whitespace fixups or add other non-mechanical cleanups, because it's a *lot* easier to see that a conversion like - inode->i_mtime = inode->i_ctime = CURRENT_TIME_SEC; + inode->i_mtime = inode->i_ctime = current_fs_time(inode); makes no other changes, but if you start doing line-splitting or other transformations (add new variables etc to get at 'sb'), suddenly you have to verify the patch at a completely different level. In other words, it's actually really important to make these kinds of bulk changes be very very obvious. Including to the point of making them visually easier to scan as a patch by not making any other changes. Linus ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH 02/21] fs: ext4: Use current_fs_time() for inode timestamps
On Thu, Jun 9, 2016 at 11:45 AM, Linus Torvalds wrote: > > All existing users and all the ones in this patch (and the others too, > although I didn't go through them very carefully) really would prefer > just passing in the inode directly, rather than the superblock. Actually, there seems to be one exception to that "all existing users", and that one exception (btrfs transacation time) really seems to be broken. Exactly because it's *not* setting an inode time, it shouldn't have used current_fs_time() to begin with, because it is just setting an internal filesystem timestamp. So not making the argument inode-related seems to actually encourage people to misuse this function. Linus ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
Re: [Y2038] [PATCH 02/21] fs: ext4: Use current_fs_time() for inode timestamps
On Wed, Jun 8, 2016 at 10:04 PM, Deepa Dinamani wrote: > CURRENT_TIME_SEC and CURRENT_TIME are not y2038 safe. > current_fs_time() will be transitioned to be y2038 safe > along with vfs. > > current_fs_time() returns timestamps according to the > granularities set in the super_block. All existing users and all the ones in this patch (and the others too, although I didn't go through them very carefully) really would prefer just passing in the inode directly, rather than the superblock. So I don't want to add more users of this broken interface. It was a mistake to use the superblock. The fact that the time granularity exists there is pretty much irrelevant. If every single user wants to use an inode pointer, then that is what the function should get. Linus ___ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038
