lkml.org 
[lkml]   [2006]   [Oct]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1 of 2] Stackfs: Introduce stackfs_copy_{attr,inode}_*
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@cs.sunysb.edu> 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@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: 2006-10-13 22:13    [W:0.066 / U:8.368 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site