Messages in this thread | | | Date | Wed, 7 Apr 2004 08:46:37 +0200 | From | Herbert Poetzl <> | Subject | Re: [Patch] BME, noatime and nodiratime |
| |
On Tue, Apr 06, 2004 at 09:48:44PM +0100, viro@parcelfarce.linux.theplanet.co.uk 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@vger.kernel.org > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |