On Fri, Oct 13, 2006 at 12:27:06PM -0700, Andrew Morton wrote:
> On Fri, 13 Oct 2006 07:18:49 -0400
> Josef "Jeff" Sipek <jsipek@xxxxxxxxxxxxx> wrote:
> > include/linux/stack_fs.h | 65 ++++++++++++++++++++++++++++++++++++++++++++++
>
> The name stack_fs implies that there's a filesystem called stackfs. Only
> there isn't. I wonder if we can choose a better name for all of this.
> Maybe fs_stack_*?
fs_stack_* sounds good to me.
> What are the locking requirements for these functions? Presumably the
> caller must hold i_mutex on at least the source inode, and perhaps the
> destination one?
>
> If i_mutex is held, i_size_read() isn't needed.
>
> If i_mutex is held, i_size_write() isn't needed either.
>
> So please document the locking requirements via source comments and then
> see if this can be simplified.
Fair enough. I'll submit an updated version.
> > +static inline void __stackfs_copy_attr_all(struct inode *dest,
> > + const struct inode *src,
> > + int (*get_nlinks)(struct inode *))
> > +{
> > + if (!get_nlinks)
> > + dest->i_nlink = src->i_nlink;
> > + else
> > + dest->i_nlink = get_nlinks(dest);
>
> I cannot find a get_nlinks() in 2.6.19-rc2?
It is the last argument to the function. Perhaps the function name is
deceiving.
> Many of these functions are too large to be inlined. Suggest they be
> placed in fs/fs-stack.c (or whatever we call it).
Ack. As a rule of thumb, for functions like these (laundry list of
assignments), what's a good threshold?
> The functions themselves seem a bit arbitrary.
> stackfs_copy_attr_timesizes() copy the three timestamps and the size. Is
> there actually any methodical reason for that, or is it simply some
> sequence which happens to have been observed in ecryptfs?
These functions come from the FiST templates [1]. Some of these can
definitely removed, or split up.
> And please - if I asked these questions when reviewing the patch, others
> will ask them when reading the code two years from now. So please treat my
> questions as "gosh, I should have put a comment in there".
That's exactly how I look at it.
Josef "Jeff" Sipek.
--
Bad pun of the week: The formula 1 control computer suffered from a race
condition
-
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
