Re: [Patch] BME, noatime and nodiratime

[Herbert Poetzl <herbert@13thfloor.at>]

On Wed, Apr 07, 2004 at 09:47:22AM +0100, viro@parcelfarce.linux.theplanet.co.uk wrote:
> On Wed, Apr 07, 2004 at 08:46:37AM +0200, Herbert Poetzl wrote:
> > originally there was a 'comment' which said, the
> > (m) check can be removed, when we are sure that this
> > isn't called with mnt == NULL ...
> > 
> > so maybe a BUGON(!m) might be useful? 
> 
> Accessing m->mnt_flags will do just fine ;-)

right ;)

> > > Note that we don't need to keep MS_NOATIME check in update_atime() - that
> > > animal is purely per-mountpoint now.
> > 
> > hmm, wasn't there a reason, for having them per inode
> > like for 'special' files, which I do not remember atm?
> 
> Yes, but that's S_NOATIME in inode->i_flags, not MS_NOATIME in sb->s_flags.
> Actually, I've been wrong here - we *do* need that check, since there are
> filesystems that force noatime or nodiratime.

so we keep them?

> >From grepping for that stuff it appears that
> 
> a) a bunch of filesystems force nodiratime and do not allow to override it
> on remount.  Same for noatime (BTW, noatime implies nodiratime, so setting
> both is pointless).
> 
> b) some filesystems force nodiratime at mount time, but do not care to preserve
> it on remount.  Most of those are my fault - I've missed the remount side of
> things in readdir patch.  They should have nodiratime always on to match
> the original behaviour.  IMO, both (a) and (b) should be handled by a new
> field - sb->s_forced_flags.  do_remount_sb() would set those in flags before
> doing anything else.  Note that assignment to ->s_flags in do_remount_sb()
> is not safe - e.g. ext[23] can get forced r/o by fs error and if that
> happens after return from ->remount_sb() but before assignement, we are
> screwed.  IOW, do_remount_sb() will need more work.

are you going to do this?

> c) XFS has an odd wankfest around MS_NOATIME - grep for XFSMNT_NOATIME and
> XFS_MOUNT_NOATIME.  AFAICS the only use is in xfs_ichgtime() and it's
> redundant - we check IS_NOATIME() right after checking for XFS_MOUNT_NOATIME
> there.  However, that will become very interesting when it gets to
> per-mountpoint r/o
>         /*
>          * We're not supposed to change timestamps in readonly-mounted
>          * filesystems.  Throw it away if anyone asks us.
>          */
>         if (unlikely(vp->v_vfsp->vfs_flag & VFS_RDONLY))
>                 return;
> in xfs_ichgtime() is too damn deep in call chains, so...
> 
> d) nfs_getattr() is very odd - it overloads semantics of noatime and nodiratime
> for NFS in a strange way.

open issues:

>>> a) what's the point of reordering them (rdonly shifting the existing ones)?

>> simple, to match the MS_* counterparts, something which
>> actually confused me in the first place (in the code)

so is this okay? actually I'd prefer to use the same
values for the MNT_NOATIME and MNT_NODIRATIME too ...
(as in the previous version I did)

best,
Herbert