Messages in this thread | | | Date | Wed, 9 Jan 2008 23:42:50 -0800 | From | Andrew Morton <> | Subject | Re: [PATCH] track number of mnts writing to superblocks |
| |
On Wed, 02 Jan 2008 13:51:10 -0800 Dave Hansen <haveblue@us.ibm.com> wrote:
> > One of the benefits of the r/o bind mount patches is that they > make it explicit when a write to a superblock might occur. > We currently search sb->s_files when remounting rw->ro to look > for writable files. But, that search is not comprehensive, and > it is racy. This replaces that search. > > The idea is to keep a bit in each mount saying whether the > mount has any writers on it. When the bit is set the first > time, we also increment a counter in the superblock. That > sb counter is the number of mounts with that bit set and > thus, potential writers. > > The other problem is that, after we make this check for > the number of writable mounts, we need to exclude all new > writers on those mounts. We do this by requring that the > superblock mnt writer count be incremented under a > lock_super() and also holding that lock over the remount > operation. Effectively, this keeps us from *adding* to > the sb's writable mounts during a remount. > > The alternative to doing this is to do a much simpler list > of mounts for each superblock. I could also code that up > to see what it look like. Shouldn't be too bad. > > ... > > -static inline void __clear_mnt_count(struct mnt_writer *cpu_writer) > +static int mark_mnt_has_writer(struct vfsmount *mnt, > + struct mnt_writer *cpu_writer) > +{ > + /* > + * Ensure that if there are people racing to set > + * the bit that only one of them succeeds and can > + * increment the sb count. > + */ > + if (test_and_set_bit(ilog2(MNT_MAY_HAVE_WRITERS), &mnt->mnt_flags)) > + return 0;
boggle.
a) bitops are to be used on unsigned long only, and mnt_flags is int.
b) nowhere else is mnt_flags used as a bitfield. Presumably (hopefully) it already has some locking. Suggest that the same locking be used here.
c) Given that all other modifiers of mnt_flags are using non-atomic rmw's, they can trash this function's rmw. What happens then?
> +static void __mark_sb_if_writable(struct vfsmount *mnt) > +{ > + int bitnr = ilog2(MNT_MAY_HAVE_WRITERS); > + > + if (atomic_read(&mnt->__mnt_writers)) > + mark_mnt_has_writer(mnt, NULL); > + else if (test_and_clear_bit(bitnr, &mnt->mnt_flags)) > + atomic_dec(&mnt->mnt_sb->__s_mnt_writers); > +}
and elsewhere.
I'm a bit surprised to see such a thing coming from your direction, Dave.
| |