lkml.org 
[lkml]   [2004]   [Apr]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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/

\
 
 \ /
  Last update: 2005-03-22 14:02    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site