[PATCH] proc: update i_atime when reading files

[PATCH] proc: update i_atime when reading files

[Jeremy Cline]

Prior to commit 1da4d377f943 ("proc: revalidate misc dentries"), the
access, modify, and change times of files in /proc were just the current
time. Now the mtime and ctime values change mostly as a user would
expect, but the atime isn't updated when the file read. This patch
updates the access time of /proc files when they are read.

Reported-by: David Both <dboth@millennium-technology.com>
Signed-off-by: Jeremy Cline <jcline@redhat.com>
---
 fs/proc/inode.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index da649ccd6804..34d8603b9aa1 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -221,12 +221,17 @@ static loff_t proc_reg_llseek(struct file *file, loff_t offset, int whence)
 static ssize_t proc_reg_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 {
 	ssize_t (*read)(struct file *, char __user *, size_t, loff_t *);
-	struct proc_dir_entry *pde = PDE(file_inode(file));
+	struct inode *inode = file_inode(file);
+	struct proc_dir_entry *pde = PDE(inode);
 	ssize_t rv = -EIO;
 	if (use_pde(pde)) {
 		read = pde->proc_fops->read;
-		if (read)
+		if (read) {
 			rv = read(file, buf, count, ppos);
+			if (rv >= 0)
+				inode->i_atime = current_time(inode);
+		}
+
 		unuse_pde(pde);
 	}
 	return rv;
-- 
2.20.1

Re: [PATCH] proc: update i_atime when reading files

[Alexey Dobriyan]

On Thu, Feb 21, 2019 at 11:37:14AM -0500, Jeremy Cline wrote:
> Prior to commit 1da4d377f943 ("proc: revalidate misc dentries"), the
> access, modify, and change times of files in /proc were just the current
> time.

Ehh, actually no. Doing

	$(which sleep) infinity </proc/foo &

will sabotage atime updates because dentry and inode will be pinned in
caches.

"revalidate misc denries" commit simply makes the effect (much) more
visible by making objects stay in caches for longer.

> Now the mtime and ctime values change mostly as a user would
> expect, but the atime isn't updated when the file read. This patch
> updates the access time of /proc files when they are read.

>  			rv = read(file, buf, count, ppos);
> +			if (rv >= 0)
> +				inode->i_atime = current_time(inode);
> +		}

Maybe it should be done given /proc is virtual so there are no concerns
about scheduling writes noone cares about to the filesystem.

Re: [PATCH] proc: update i_atime when reading files

[Jeremy Cline]

On Fri, Feb 22, 2019 at 08:37:42AM +0300, Alexey Dobriyan wrote:
> On Thu, Feb 21, 2019 at 11:37:14AM -0500, Jeremy Cline wrote:
> > Prior to commit 1da4d377f943 ("proc: revalidate misc dentries"), the
> > access, modify, and change times of files in /proc were just the current
> > time.
> 
> Ehh, actually no. Doing
> 
> 	$(which sleep) infinity </proc/foo &
> 
> will sabotage atime updates because dentry and inode will be pinned in
> caches.
> 
> "revalidate misc denries" commit simply makes the effect (much) more
> visible by making objects stay in caches for longer.

Indeed. It wasn't my intention to imply there's anything wrong with that
commit, just that that's what caused this apparent change in behavior
for users. In the "common" case when something hasn't pinned the dentry
and inode what users saw was the current time.

> 
> > Now the mtime and ctime values change mostly as a user would
> > expect, but the atime isn't updated when the file read. This patch
> > updates the access time of /proc files when they are read.
> 
> >  			rv = read(file, buf, count, ppos);
> > +			if (rv >= 0)
> > +				inode->i_atime = current_time(inode);
> > +		}
> 
> Maybe it should be done given /proc is virtual so there are no concerns
> about scheduling writes noone cares about to the filesystem.

Sorry, maybe I've not had enough coffee yet, but I don't understand this
sentence.

Thanks,
Jeremy

Re: [PATCH] proc: update i_atime when reading files

[Alexey Dobriyan]

On Fri, Feb 22, 2019 at 10:00:20AM -0500, Jeremy Cline wrote:
> On Fri, Feb 22, 2019 at 08:37:42AM +0300, Alexey Dobriyan wrote:
> > On Thu, Feb 21, 2019 at 11:37:14AM -0500, Jeremy Cline wrote:
> > > Prior to commit 1da4d377f943 ("proc: revalidate misc dentries"), the
> > > access, modify, and change times of files in /proc were just the current
> > > time.
> > 
> > Ehh, actually no. Doing
> > 
> > 	$(which sleep) infinity </proc/foo &
> > 
> > will sabotage atime updates because dentry and inode will be pinned in
> > caches.
> > 
> > "revalidate misc denries" commit simply makes the effect (much) more
> > visible by making objects stay in caches for longer.
> 
> Indeed. It wasn't my intention to imply there's anything wrong with that
> commit, just that that's what caused this apparent change in behavior
> for users. In the "common" case when something hasn't pinned the dentry
> and inode what users saw was the current time.
> 
> > 
> > > Now the mtime and ctime values change mostly as a user would
> > > expect, but the atime isn't updated when the file read. This patch
> > > updates the access time of /proc files when they are read.
> > 
> > >  			rv = read(file, buf, count, ppos);
> > > +			if (rv >= 0)
> > > +				inode->i_atime = current_time(inode);
> > > +		}
> > 
> > Maybe it should be done given /proc is virtual so there are no concerns
> > about scheduling writes noone cares about to the filesystem.
> 
> Sorry, maybe I've not had enough coffee yet, but I don't understand this
> sentence.

I meant it should be harmless to enable atime unconditionally for /proc
because it is virtual filesystem.