Re: [Patch] BME, noatime and nodiratime

[Herbert Poetzl]

On Tue, Apr 06, 2004 at 09:48:44PM +0100, viro@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx wrote:
> On Tue, Apr 06, 2004 at 04:55:44PM +0200, Herbert Poetzl wrote:
> > 
> > Hi Andrew!
> > 
> > according to todays vfs strategy (hope it hasn't changed
> > again), here is the first patch, which adds the mount 
> > flags propagation, fixes the /proc display, and implements
> > noatime and nodiratime per mountpoint ...
> > 
> > please consider for inclusion ...
> 
> noatime/nodiratime: OK, but we still have direct modifications of i_atime
> that need to be taken care of.

okay, will look at it ...

> massage of ->show(): more or less OK.  However, we don't need to keep
> MS_NOATIME and MS_NODIRATIME in flags at all - 
> > +	if (flags & MS_NOATIME)
> > +		mnt_flags |= MNT_NOATIME;
> > +	if (flags & MS_NODIRATIME)
> > +		mnt_flags |= MNT_NODIRATIME;
> >  	flags &= ~(MS_NOSUID|MS_NOEXEC|MS_NODEV);
> 
> should remove them from flags in the last line, same way we do that for
> nosuid/noexec/nodev, with obvious consequences for ->show().
> 
> 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?

> > +	if (MNT_IS_NOATIME(mnt))
> > +		return;
> > +	if (S_ISDIR(inode->i_mode) && MNT_IS_NODIRATIME(mnt))
> > +		return;
> 
> Do we need those to be macros?  AFAICS, this is the only place where we
> do such checks and we shouldn't get new callers.  IOW, keeping them
> separate doesn't buy us anything and only obfuscates the code.

okay, no problem with that ...

> > -#define MNT_NOSUID	1
> > -#define MNT_NODEV	2
> > -#define MNT_NOEXEC	4
> > +#define MNT_RDONLY	1
> > +#define MNT_NOSUID	2
> > +#define MNT_NODEV	4
> > +#define MNT_NOEXEC	8
> > +#define MNT_NOATIME	16
> > +#define MNT_NODIRATIME	32
> 
> *ugh*
> 
> 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)

> b) since MNT_RDONLY doesn't do anything at that point, why introduce it
> (and associated confusion) now?  As it is, your /proc/mounts will pretend
> that per-mountpoint r/o works right now.  Since it doesn't...

no, they won't. the required MNT_RDONLY flag in the proc
function isn't set, so only MS_RDONLY will be reported
(from the superblock) which is equivalent to what is 
reported in the current version.

> > +#define	MNT_IS_RDONLY(m)	((m) && ((m)->mnt_flags & MNT_RDONLY))
> > +#define	MNT_IS_NOATIME(m)	((m) && ((m)->mnt_flags & MNT_NOATIME))
> > +#define	MNT_IS_NODIRATIME(m)	((m) && ((m)->mnt_flags & MNT_NODIRATIME))
> 
> See above.  Besides, are we ever planning to pass NULL to these guys?

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? 

I'll add the 'updates' today ...

best,
Herbert

> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/