Re: [PATCH] hurd/libdiskfs: I added in the `relatime' mount option.

[Samuel Thibault]

Hello,

Ryan Jeffrey, le jeu. 10 sept. 2020 14:01:58 -0700, a ecrit:
> Using Linux as a guide, I implemented the `relatime' mount option as 
> mentioned in the Hurd's Contributing page.

Thanks for your work! Here are a few comments inline.

Samuel

> diff -ruN a/hurd/libdiskfs/diskfs.h b/hurd/libdiskfs/diskfs.h
> --- a/hurd/libdiskfs/diskfs.h 2020-07-18 12:08:35.000000000 -0700
> +++ b/hurd/libdiskfs/diskfs.h 2020-09-09 23:49:31.000000000 -0700
> @@ -1046,7 +1046,9 @@
>  diskfs_init_dir (struct node *dp, struct node *pdp, struct protid *cred);
>  
>  /* If disk is not readonly and the noatime option is not enabled, set
> -   NP->dn_set_atime.  */
> +   NP->dn_set_atime.  If relatime is enabled, only set NP->dn_set_atime
> +   if the atime has not been updated today, or if ctime or mtime are
> +   more recent than atime */
>  void diskfs_set_node_atime (struct node *np);
>  
>  /* If NP->dn_set_ctime is set, then modify NP->dn_stat.st_ctim

Please also update the diskfs_set_node_atime documentation in
doc/hurd.texi.

> diff -ruN a/hurd/libdiskfs/node-times.c b/hurd/libdiskfs/node-times.c
> --- a/hurd/libdiskfs/node-times.c     2020-09-10 02:49:31.000000000 -0700
> +++ b/hurd/libdiskfs/node-times.c     2020-09-09 23:47:15.000000000 -0700
> @@ -24,12 +24,48 @@
>  #include "priv.h"
>  #include <maptime.h>
>  
> +/* If the disk is not readonly and noatime is not set, then check relatime
> +   conditions: if either `np->dn_stat.st_mtim.tv_sec' or
> +   `np->dn_stat.st_ctim.tv_sec' is less than `np->dn_stat.st_atim.tv_sec',

shouldn't that be "is greater"? (i.e. more recent)

> +   or if the atime is greater than 24 hours old, return true.
> +   */
> +int
> +atime_should_update (struct node *np)
> +{
> +  if (_diskfs_noatime)
> +    return 0;
> +
> +  if (_diskfs_relatime)
> +    {
> +      /* Update atime if mtime is younger than atime. */
> +      if (np->dn_stat.st_mtim.tv_sec > np->dn_stat.st_atim.tv_sec)
> +        return 1;
> +      /* Update atime if ctime is younger than atime. */
> +      else if (np->dn_stat.st_ctim.tv_sec > np->dn_stat.st_atim.tv_sec)

You can drop the else keyword.

> +        return 1;
> +      /* Update atime if current atime is more than 24 hours old. */
> +      else

You can drop the else keyword. You can leave timeval as a variable for
the whole function, the compiler will properly optimize its allocation.

Samuel