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    [W:0.026 / U:358.580 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site