Re: + embed-a-struct-path-into-struct-nameidata-instead-of-nd-dentrymnt.pa tch added to -mm tree
Junjiro Okajima, first of all thanks for the feedback on my union mount patches. On Tue, Nov 06, [EMAIL PROTECTED] wrote: > Whiteouts in your code can be a serious memory pressure, since they are > kept in dcache. I know the inode for whiteouts exists only one and it is > shared, but dentries for whiteouts are not. They are created for each > name and resident in dcache. > I am afraid it can be a problem easily when you create and unlink a > temporary file many times. Generally their filenames are unique. The problem that you describe is only existing on tmpfs as the topmost union layer. In all other cases the whiteout dentries can be shrinked like the dentries of other filetypes too. This is the price you have to pay for using union mounts because somewhere this information must be stored. With ext3 or other diskbased filesystems the whiteouts are stored on disk like normal files. Therefore the dentry cache can be shrinked and reread by a lookup. > Regarding to struct path in nameidata, I have no objection > basically. But I think it is better to create macros for backward > compatibility as struct file did. In case of f_dentry and f_mnt that was easy because you could use macros for it. Still people tend to be lazy and don't change their code if you don't force them (or do it for them). Anyway, in nameidata we used dentry and mnt as the field names. Therefore it isn't possible to use macros except of stuff like ND2DENTRY(nd) kind of stuff which is even worse. - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: + embed-a-struct-path-into-struct-nameidata-instead-of-nd-dentrymnt.pa tch added to -mm tree
> On Tue, 6 Nov 2007 12:30:38 +0100 Jörn Engel <[EMAIL PROTECTED]> wrote:
> On Tue, 6 November 2007 10:11:49 +0100, Jan Blunck wrote:
> > On Mon, Nov 05, Jörn Engel wrote:
> >
> > > This patch changes some 400 lines, most if not all of which get longer
> > > and more complicated to read. 23 get sufficiently longer to require an
> > > additional linebreak. I can't remember complexity being invited into
> > > the kernel without good reasoning, yet the patch description is
> > > surprisingly low on reasoning:
> > > > Switch from nd->{dentry,mnt} to nd->path.{dentry,mnt} everywhere.
> >
> > I don't measure complexity by lines of code or length of lines. Maybe I was
> > not verbose enough in the description, fair.
>
> If you have a better metric, please share it. In the paragraph you
> deleted I explicitly asked for _any_ metric that shows favorable
> numbers. Lacking numbers, we could only argue about our respective
> personal taste.
>
> > This is a cleanup series. In mostly no case there is a reason why someone
> > would want to use a dentry for itself. This series reflects that fact in
> > nameidata where there is absolutly no reason at all.
>
> 400+ lines changed in this patch, some 10 in a followup patch that
> combines dentry/vfsmount assignments into a single path assignment. If
> your argument above was valid, I would expect more simplifications and
> fewer complications. Call me a sceptic until further patches show up to
> support your point.
>
> > It enforced the correct
> > order of getting/releasing refcount on pairs.
>
> This argument I buy.
>
> > It enables us
> > to do some more cleanups wrt lookup (which are coming later).
>
> Please send those patches. I invite cleanups that do clean things up
> and won't argue against then. ;)
>
> > For stacking
> > support in VFS it is essential to have the pair in every
> > place where you want to traverse the stack.
>
> True, but unrelated to this patch.
>
> > > If churn is the only effect of this, please considere it NAKed again.
> >
> > I wonder why you didn't speak up when this series was posted to LKML. It was
> > at least posted three times before.
>
> I did speak up. Once. If you missed that thread, please forgive me
> missing those in which the same patch I disapproved of were resent
> without me on Cc.
>
> I'm not categorically against this struct path business. It does have
> some advantages at first glance. But the patch we're arguing about
> clearly makes code more complicated and harder to read. We should have
> more than superficial benefits if we decide to pay such a cost.
It sounds like we at least need a better overall changlog, please..
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: + embed-a-struct-path-into-struct-nameidata-instead-of-nd-dentrymnt.pa tch added to -mm tree
Hello Jan Blunck, Jan Blunck: > I start sending out the patches in multiple chunks because nobody reviewed the > union mount series except for coding style violations. So this is the prework > for the changes that come with my union mount series. So they are related > but not a part of the union mount patch series. It seems that people tend to > like the patch series with small changes for itself instead of a big fat > series. I've read your union mount code which was posted on the end of last July. Here is a comment which is I remember now. Whiteouts in your code can be a serious memory pressure, since they are kept in dcache. I know the inode for whiteouts exists only one and it is shared, but dentries for whiteouts are not. They are created for each name and resident in dcache. I am afraid it can be a problem easily when you create and unlink a temporary file many times. Generally their filenames are unique. Regarding to struct path in nameidata, I have no objection basically. But I think it is better to create macros for backward compatibility as struct file did. Junjiro Okajima - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: + embed-a-struct-path-into-struct-nameidata-instead-of-nd-dentrymnt.pa tch added to -mm tree
On Tue, 6 November 2007 13:59:03 +0100, Jan Blunck wrote: > > Coming back to the mental stuff: the savings of the first bunch of patches > that already hit -mm: > > Textsize without patches: 0x20e572 > Textsize with patches:0x20e042 > -- > 0x530 = 1328 bytes Ok, that is more substantial than mental masturbation. Thank you for the numbers. All my criticism becomes void with this. Jörn -- Victory in war is not repetitious. -- Sun Tzu - To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: + embed-a-struct-path-into-struct-nameidata-instead-of-nd-dentrymnt.pa tch added to -mm tree
On Tue, Nov 06, Jörn Engel wrote:
> > This is a cleanup series. In mostly no case there is a reason why someone
> > would want to use a dentry for itself. This series reflects that fact in
> > nameidata where there is absolutly no reason at all.
>
> 400+ lines changed in this patch, some 10 in a followup patch that
> combines dentry/vfsmount assignments into a single path assignment. If
> your argument above was valid, I would expect more simplifications and
> fewer complications. Call me a sceptic until further patches show up to
> support your point.
This are the patches currently in -mm related to the struct path cleanup:
move-struct-path-into-its-own-header.patch
embed-a-struct-path-into-struct-nameidata-instead-of-nd-dentrymnt.patch
introduce-path_put.patch
use-path_put-in-a-few-places-instead-of-mntdput.patch
introduce-path_get.patch
use-struct-path-in-fs_struct.patch
make-set_fs_rootpwd-take-a-struct-path.patch
introduce-path_get-unionfs.patch
embed-a-struct-path-into-struct-nameidata-instead-of-nd-dentrymnt-unionfs.patch
introduce-path_put-unionfs.patch
one-less-parameter-to-__d_path.patch
d_path-kerneldoc-cleanup.patch
d_path-use-struct-path-in-struct-avc_audit_data.patch
d_path-make-proc_get_link-use-a-struct-path-argument.patch
d_path-make-get_dcookie-use-a-struct-path-argument.patch
use-struct-path-in-struct-svc_export.patch
use-struct-path-in-struct-svc_expkey.patch
d_path-make-seq_path-use-a-struct-path-argument.patch
d_path-make-d_path-use-a-struct-path.patch
> > It enables us
> > to do some more cleanups wrt lookup (which are coming later).
>
> Please send those patches. I invite cleanups that do clean things up
> and won't argue against then. ;)
I'll send them in a later series.
> > For stacking
> > support in VFS it is essential to have the pair in every
> > place where you want to traverse the stack.
>
> True, but unrelated to this patch.
I start sending out the patches in multiple chunks because nobody reviewed the
union mount series except for coding style violations. So this is the prework
for the changes that come with my union mount series. So they are related
but not a part of the union mount patch series. It seems that people tend to
like the patch series with small changes for itself instead of a big fat
series.
> > I wonder why you didn't speak up when this series was posted to LKML. It was
> > at least posted three times before.
>
> I did speak up. Once. If you missed that thread, please forgive me
> missing those in which the same patch I disapproved of were resent
> without me on Cc.
Sorry for missing your feedback but now I found your mail ("mental
masturbation that complicates the source"). I guess this is what happens when
multiple people start posting the same patch series.
Coming back to the mental stuff: the savings of the first bunch of patches
that already hit -mm:
Textsize without patches: 0x20e572
Textsize with patches:0x20e042
--
0x530 = 1328 bytes
Cheers,
Jan
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: + embed-a-struct-path-into-struct-nameidata-instead-of-nd-dentrymnt.pa tch added to -mm tree
On Tue, 6 November 2007 10:11:49 +0100, Jan Blunck wrote:
> On Mon, Nov 05, Jörn Engel wrote:
>
> > This patch changes some 400 lines, most if not all of which get longer
> > and more complicated to read. 23 get sufficiently longer to require an
> > additional linebreak. I can't remember complexity being invited into
> > the kernel without good reasoning, yet the patch description is
> > surprisingly low on reasoning:
> > > Switch from nd->{dentry,mnt} to nd->path.{dentry,mnt} everywhere.
>
> I don't measure complexity by lines of code or length of lines. Maybe I was
> not verbose enough in the description, fair.
If you have a better metric, please share it. In the paragraph you
deleted I explicitly asked for _any_ metric that shows favorable
numbers. Lacking numbers, we could only argue about our respective
personal taste.
> This is a cleanup series. In mostly no case there is a reason why someone
> would want to use a dentry for itself. This series reflects that fact in
> nameidata where there is absolutly no reason at all.
400+ lines changed in this patch, some 10 in a followup patch that
combines dentry/vfsmount assignments into a single path assignment. If
your argument above was valid, I would expect more simplifications and
fewer complications. Call me a sceptic until further patches show up to
support your point.
> It enforced the correct
> order of getting/releasing refcount on pairs.
This argument I buy.
> It enables us
> to do some more cleanups wrt lookup (which are coming later).
Please send those patches. I invite cleanups that do clean things up
and won't argue against then. ;)
> For stacking
> support in VFS it is essential to have the pair in every
> place where you want to traverse the stack.
True, but unrelated to this patch.
> > If churn is the only effect of this, please considere it NAKed again.
>
> I wonder why you didn't speak up when this series was posted to LKML. It was
> at least posted three times before.
I did speak up. Once. If you missed that thread, please forgive me
missing those in which the same patch I disapproved of were resent
without me on Cc.
I'm not categorically against this struct path business. It does have
some advantages at first glance. But the patch we're arguing about
clearly makes code more complicated and harder to read. We should have
more than superficial benefits if we decide to pay such a cost.
> Did I break your COW link patches? ;)
Nope. No bovine maladies involved.
Jörn
--
The competent programmer is fully aware of the strictly limited size of
his own skull; therefore he approaches the programming task in full
humility, and among other things he avoids clever tricks like the plague.
-- Edsger W. Dijkstra
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: + embed-a-struct-path-into-struct-nameidata-instead-of-nd-dentrymnt.pa tch added to -mm tree
On Mon, Nov 05, Jörn Engel wrote:
> > Subject: Embed a struct path into struct nameidata instead of
> > nd->{dentry,mnt}
> > From: Jan Blunck <[EMAIL PROTECTED]>
> >
> > Switch from nd->{dentry,mnt} to nd->path.{dentry,mnt} everywhere.
> >
> > Signed-off-by: Jan Blunck <[EMAIL PROTECTED]>
> > Signed-off-by: Andreas Gruenbacher <[EMAIL PROTECTED]>
> > Acked-by: Christoph Hellwig <[EMAIL PROTECTED]>
> > Cc: Al Viro <[EMAIL PROTECTED]>
> > CC:
> > Signed-off-by: Andrew Morton <[EMAIL PROTECTED]>
> Frowned-upon-by: Joern Engel <[EMAIL PROTECTED]>
>
> This patch changes some 400 lines, most if not all of which get longer
> and more complicated to read. 23 get sufficiently longer to require an
> additional linebreak. I can't remember complexity being invited into
> the kernel without good reasoning, yet the patch description is
> surprisingly low on reasoning:
> > Switch from nd->{dentry,mnt} to nd->path.{dentry,mnt} everywhere.
I don't measure complexity by lines of code or length of lines. Maybe I was
not verbose enough in the description, fair.
This is a cleanup series. In mostly no case there is a reason why someone
would want to use a dentry for itself. This series reflects that fact in
nameidata where there is absolutly no reason at all. It enforced the correct
order of getting/releasing refcount on pairs. It enables us
to do some more cleanups wrt lookup (which are coming later). For stacking
support in VFS it is essential to have the pair in every
place where you want to traverse the stack.
> If churn is the only effect of this, please considere it NAKed again.
I wonder why you didn't speak up when this series was posted to LKML. It was
at least posted three times before.
Did I break your COW link patches? ;)
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
