Re: [PATCH 1 of 2] Stackfs: Introduce stackfs_copy_{attr,inode}_*

[Josef Sipek]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/