Messages in this thread | | | Date | Fri, 22 Jul 2022 17:17:09 +0800 | Subject | Re: [RFC] proc: fix create timestamp of files in proc | From | Yuchen Zhang <> |
| |
Maybe I have some problem with my presentation, sorry.
I actually don't think any of these three suggestions are a good idea. But I don't know if there is a better solution. This RFC just wants to raise the question.
Thanks for your replies.
On 2022/7/22 11:43, Muchun Song wrote: > On Fri, Jul 22, 2022 at 12:16 AM Luis Chamberlain <mcgrof@kernel.org> wrote: >> >> On Thu, Jul 21, 2022 at 04:16:17PM +0800, Zhang Yuchen wrote: >>> A user has reported a problem that the /proc/{pid} directory >>> creation timestamp is incorrect. >> >> The directory? >> >>> He believes that the directory was created when the process was >>> started, so the timestamp of the directory creation should also >>> be when the process was created. >> >> A quick glance at Documentation/filesystems/proc.rst reveals there >> is documentation that the process creation time is the start_time in >> the stat file for the pid. It makes absolutely no mention of the >> directory creation time. >> > > Hi Luis, > > Right. > >> The directory creation time has been the way it is since linux history [0] >> commit fdb2f0a59a1c7 ("[PATCH] Linux-0.97.3 (September 5, 1992)) and so this >> has been the way .. since the beginning. >> >> [0] https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/ >> >> The last change was by Deepa to correct y2038 considerations through >> commit 078cd8279e659 ("fs: Replace CURRENT_TIME with current_time() for >> inode timestamps"). >> >> Next time you try to report something like this please be very sure >> to learn to use git blame, and then git blame foo.c <commit-id>~1 and >> keep doing this until you get to the root commit, this will let you >> determine *how long has this been this way*. When you run into a >> commit history which lands to the first git commit on linux you can >> use the above linux history.git to go back further as I did. >> > > A good instruction for a newbie. > >>> The file timestamp in procfs is the timestamp when the inode was >>> created. If the inode of a file in procfs is reclaimed, the inode >>> will be recreated when it is opened again, and the timestamp will >>> be changed to the time when it was recreated. >> >> The commit log above starts off with a report of the directory >> of a PID. When does the directory of a PID change dates when its >> respective start_time does not? When does this reclaim happen exactly? >> Under what situation? >> > > IMHO, when the system is under memory pressure, then the proc > inode can be reclaimed, it is also true when we `echo 3 > > /proc/sys/vm/drop_caches`. After this operation, the proc inode's > timestamp is changed. > >> And if that is not happening, can you name *one* file in a process >> directory under proc which does get reclaimed for, for which this >> does happen? >> >>> In other file systems, this timestamp is typically recorded in >>> the file system and assigned to the inode when the inode is created. >> >> I don't understand, which files are we reclaiming in procfs which >> get re-recreated which your *user* is having issues with? What did >> they report exactly, I'm *super* curious what your user reported >> exactly. Do you have a bug report somewhere? Or any information >> about its bug report. Can you pass it on to Muchun for peer review? >> What file were they monitoring and what tool were they using which >> made them realize there was a sort of issue? >> >>> This mechanism can be confusing to users who are not familiar with it. >> >> Why are they monitoring it? Why would a *new* inode having a different >> timestamp be an issue as per existing documentation? >> > > Maybe the users think the timestamp of /proc/$pid directory is equal to > the start_time of a process, I think it is because of a comment of > shortage about the meaning of the timestamp of /proc files. > >>> For users who know this mechanism, they will choose not to trust this time. >>> So the timestamp now has no meaning other than to confuse the user. >> >> That is unfair given this is the first *user* to report confusion since >> the inception of Linux, don't you think? >> >>> It needs fixing. >> >> A fix is for when there is an issue. You are not reporting a bug or an >> issue, but you seem to be reporting something a confused user sees and >> perhaps lack of documentation for something which is not even tracked >> or cared for. This is the way things have been done since the beginning. >> It doesn't mean things can't change, but there needs to be a good reason. >> >> The terminology of "fix" implies something is broken. The only thing >> seriouly broken here is this patch you are suggesting and the mechanism >> which is enabling you to send patches for what you think are issues and >> seriously wasting people's time. That seriously needs to be fixed. >> >>> There are three solutions. We don't have to make the timestamp >>> meaningful, as long as the user doesn't trust the timestamp. >>> >>> 1. Add to the kernel documentation that the timestamp in PROC is >>> not reliable and do not use this timestamp. >>> The problem with this solution is that most users don't read >>> the kernel documentation and it can still be misleading. >>> >>> 2. Fix it, change the timestamp of /proc/pid to the timestamp of >>> process creation. >>> >>> This raises new questions. >>> >>> a. Users don't know which kernel version is correct. >>> >>> b. This problem exists not only in the pid directory, but also >>> in other directories under procfs. It would take a lot of >>> extra work to fix them all. There are also easier ways for >>> users to get the creation time information better than this. >>> >>> c. We need to describe the specific meaning of each file under >>> proc in the kernel documentation for the creation time. >>> Because the creation time of various directories has different >>> meanings. For example, PID directory is the process creation >>> time, FD directory is the FD creation time and so on. >>> >>> d. Some files have no associated entity, such as iomem. >>> Unable to give a meaningful time. >>> >>> 3. Instead of fixing it, set the timestamp in all procfs to 0. >>> Users will see it as an error and will not use it. >>> >>> I think 3 is better. Any other suggestions? >>> >>> Signed-off-by: Zhang Yuchen <zhangyuchen.lcr@bytedance.com> >> >> The logic behind this patch is way off track, a little effort >> alone should have made you reach similar conflusions as I have. >> Your patch does your suggested step 3), so no way! What you are >> proposing can potentially break things! Have you put some effort >> into evaluating the negative possible impacts of your patch? If >> not, can you do that now? Did you even *boot* test your patch? >> >> It makes all of the proc files go dated back to Jan 1 1970. >> >> How can this RFC in any way shape or form have been sent with >> a serious intent? >> >> Sadly the lack of any serious consideration of the past and then >> for you to easily suggest to make a new change which could easily >> break existing users makes me needing to ask you to please have >> one of your peers at bytedance.com such as Muchun Song to please >> review your patches prior to you posting them, because otherwise >> this is creating noise and quite frankly make me wonder if you >> are intentially trying to break things. >> >> Muchun Song, sorry but can you please help here ensure that your >> peers don't post this level of quality of patches again? It would be >> seriously appreciated. >> > > It's my bad. Sorry. I should review it carefully. Zhang Yuchen is a newbie > for Linux kernel development, I think he just want to point out the potential > confusion to the users. Yuchen is not sure if this change is the best, I suspect > this is why there is RFC is the patch's subject. I think at least we > should point > it out in Documentation/filesystems/proc.rst so that users can know what does > the timestamp of proc files/directories mean. > > Thanks. > >> Users exist for years without issue and now you want to change things >> for a user which finds something done which is not documented and want >> to purposely *really* change things for *everyone* to ways which have >> 0 compatibility with what users may have been expecting before. >> >> How can you conclude this? >> >> This suggested patch is quite alarming. >> >> Luis >> >> Below is just nonsense. >> >>> --- >>> fs/proc/base.c | 4 +++- >>> fs/proc/inode.c | 3 ++- >>> fs/proc/proc_sysctl.c | 3 ++- >>> fs/proc/self.c | 3 ++- >>> fs/proc/thread_self.c | 3 ++- >>> 5 files changed, 11 insertions(+), 5 deletions(-) >>> >>> diff --git a/fs/proc/base.c b/fs/proc/base.c >>> index 0b72a6d8aac3..af440ef13091 100644 >>> --- a/fs/proc/base.c >>> +++ b/fs/proc/base.c >>> @@ -1892,6 +1892,8 @@ struct inode *proc_pid_make_inode(struct super_block *sb, >>> struct proc_inode *ei; >>> struct pid *pid; >>> >>> + struct timespec64 ts_zero = {0, 0}; >>> + >>> /* We need a new inode */ >>> >>> inode = new_inode(sb); >>> @@ -1902,7 +1904,7 @@ struct inode *proc_pid_make_inode(struct super_block *sb, >>> ei = PROC_I(inode); >>> inode->i_mode = mode; >>> inode->i_ino = get_next_ino(); >>> - inode->i_mtime = inode->i_atime = inode->i_ctime = current_time(inode); >>> + inode->i_mtime = inode->i_atime = inode->i_ctime = ts_zero; >>> inode->i_op = &proc_def_inode_operations; >>> >>> /* >>> diff --git a/fs/proc/inode.c b/fs/proc/inode.c >>> index fd40d60169b5..efb1c935fa8d 100644 >>> --- a/fs/proc/inode.c >>> +++ b/fs/proc/inode.c >>> @@ -642,6 +642,7 @@ const struct inode_operations proc_link_inode_operations = { >>> struct inode *proc_get_inode(struct super_block *sb, struct proc_dir_entry *de) >>> { >>> struct inode *inode = new_inode(sb); >>> + struct timespec64 ts_zero = {0, 0}; >>> >>> if (!inode) { >>> pde_put(de); >>> @@ -650,7 +651,7 @@ struct inode *proc_get_inode(struct super_block *sb, struct proc_dir_entry *de) >>> >>> inode->i_private = de->data; >>> inode->i_ino = de->low_ino; >>> - inode->i_mtime = inode->i_atime = inode->i_ctime = current_time(inode); >>> + inode->i_mtime = inode->i_atime = inode->i_ctime = ts_zero; >>> PROC_I(inode)->pde = de; >>> if (is_empty_pde(de)) { >>> make_empty_dir_inode(inode); >>> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c >>> index 021e83fe831f..c670f9d3b871 100644 >>> --- a/fs/proc/proc_sysctl.c >>> +++ b/fs/proc/proc_sysctl.c >>> @@ -455,6 +455,7 @@ static struct inode *proc_sys_make_inode(struct super_block *sb, >>> struct ctl_table_root *root = head->root; >>> struct inode *inode; >>> struct proc_inode *ei; >>> + struct timespec64 ts_zero = {0, 0}; >>> >>> inode = new_inode(sb); >>> if (!inode) >>> @@ -476,7 +477,7 @@ static struct inode *proc_sys_make_inode(struct super_block *sb, >>> head->count++; >>> spin_unlock(&sysctl_lock); >>> >>> - inode->i_mtime = inode->i_atime = inode->i_ctime = current_time(inode); >>> + inode->i_mtime = inode->i_atime = inode->i_ctime = ts_zero; >>> inode->i_mode = table->mode; >>> if (!S_ISDIR(table->mode)) { >>> inode->i_mode |= S_IFREG; >>> diff --git a/fs/proc/self.c b/fs/proc/self.c >>> index 72cd69bcaf4a..b9e572fdc27c 100644 >>> --- a/fs/proc/self.c >>> +++ b/fs/proc/self.c >>> @@ -44,9 +44,10 @@ int proc_setup_self(struct super_block *s) >>> self = d_alloc_name(s->s_root, "self"); >>> if (self) { >>> struct inode *inode = new_inode(s); >>> + struct timespec64 ts_zero = {0, 0}; >>> if (inode) { >>> inode->i_ino = self_inum; >>> - inode->i_mtime = inode->i_atime = inode->i_ctime = current_time(inode); >>> + inode->i_mtime = inode->i_atime = inode->i_ctime = ts_zero; >>> inode->i_mode = S_IFLNK | S_IRWXUGO; >>> inode->i_uid = GLOBAL_ROOT_UID; >>> inode->i_gid = GLOBAL_ROOT_GID; >>> diff --git a/fs/proc/thread_self.c b/fs/proc/thread_self.c >>> index a553273fbd41..964966387da2 100644 >>> --- a/fs/proc/thread_self.c >>> +++ b/fs/proc/thread_self.c >>> @@ -44,9 +44,10 @@ int proc_setup_thread_self(struct super_block *s) >>> thread_self = d_alloc_name(s->s_root, "thread-self"); >>> if (thread_self) { >>> struct inode *inode = new_inode(s); >>> + struct timespec64 ts_zero = {0, 0}; >>> if (inode) { >>> inode->i_ino = thread_self_inum; >>> - inode->i_mtime = inode->i_atime = inode->i_ctime = current_time(inode); >>> + inode->i_mtime = inode->i_atime = inode->i_ctime = ts_zero; >>> inode->i_mode = S_IFLNK | S_IRWXUGO; >>> inode->i_uid = GLOBAL_ROOT_UID; >>> inode->i_gid = GLOBAL_ROOT_GID; >>> -- >>> 2.30.2 >>>
| |