In message <20091203142329.46cdcc44.akpm@xxxxxxxxxxxxxxxxxxxx>, Andrew Morton writes: > On Wed, 2 Dec 2009 19:21:55 -0500 > Erez Zadok <ezk@xxxxxxxxxxxxx> wrote: > > > > > Andrew, > > > > This is a patch I've been using successfully in Unionfs for more than a year > > and multiple kernel versions. The origin of patch was from Hugh Dickins, > > who found problems with 32-bit/SMP systems. You and hch had some comments. > > This is the final version which I've been using. See thread of discussion > > starting here: > > > > <http://marc.info/?l=linux-kernel&m=121469807803867&w=2> > > > > The comments in this patch provide more details. I think this patch is > > ready for upstream. At the moment, the only mainline user of this code is > > ecryptfs. > > > > um. None of that is usable as a changelog. Will fix. > > diff --git a/fs/stack.c b/fs/stack.c > > index 67716f6..3d7f5c3 100644 > > --- a/fs/stack.c > > +++ b/fs/stack.c > > @@ -9,8 +9,54 @@ > > */ > > void fsstack_copy_inode_size(struct inode *dst, const struct inode *src) > > { > > - i_size_write(dst, i_size_read((struct inode *)src)); > > - dst->i_blocks = src->i_blocks; > > + loff_t i_size; > > + blkcnt_t i_blocks; > > + > > + /* > > + * i_size_read() includes its own seqlocking and protection from > > + * preemption (see include/linux/fs.h): we need nothing extra for > > + * that here, and prefer to avoid nesting locks than attempt to > > + * keep i_size and i_blocks in synch together. > > + */ > > + i_size = i_size_read(src); > > + > > + /* > > + * But if CONFIG_LSF (on 32-bit), we ought to make an effort to keep > > CONFIG_LBDAF? Ah, yes, the CONFIG option name has changed. Thanks. I assume you meant teh comment is incorrect. > > + * the two halves of i_blocks in synch despite SMP or PREEMPT - though > > + * stat's generic_fillattr() doesn't bother, and we won't be applying > > + * quotas (where i_blocks does become important) at the upper level. > > + * > > + * We don't actually know what locking is used at the lower level; but > > + * if it's a filesystem that supports quotas, it will be using i_lock > > + * as in inode_add_bytes(). tmpfs uses other locking, and its 32-bit > > + * is (just) able to exceed 2TB i_size with the aid of holes; but its > > + * i_blocks cannot carry into the upper long without almost 2TB swap - > > + * let's ignore that case. > > + */ > > + if (sizeof(i_blocks) > sizeof(long)) > > + spin_lock((spinlock_t *) &src->i_lock); > > + i_blocks = src->i_blocks; > > + if (sizeof(i_blocks) > sizeof(long)) > > + spin_unlock((spinlock_t *) &src->i_lock); > > And the typecasts are needed because `src' is const. urgh. Logically > true but practically false. Perhaps just remove the const? I agree. It's not much help here. I'll remove the const from the function proto. The alternative would be to propagate the const down to spin_{un}lock(), but I think it's best not to touch such a critical function for the sake of seldom used fsstack code. > > + /* > > + * If CONFIG_SMP on 32-bit, it's vital for fsstack_copy_inode_size() > > And CONFIG_PREEMPT. Will fix the comment here too. Erez. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
