Allocate strings with kmalloc.
Checkstack output:
Before: udf_get_filename: 600
After: udf_get_filename: 136
Signed-off-by: Marcin Slusarz <[email protected]>
Cc: Jan Kara <[email protected]>
---
fs/udf/unicode.c | 41 +++++++++++++++++++++++++----------------
1 files changed, 25 insertions(+), 16 deletions(-)
diff --git a/fs/udf/unicode.c b/fs/udf/unicode.c
index 9fdf8c9..a3bbdbd 100644
--- a/fs/udf/unicode.c
+++ b/fs/udf/unicode.c
@@ -324,34 +324,43 @@ try_again:
int udf_get_filename(struct super_block *sb, uint8_t *sname, uint8_t *dname,
int flen)
{
- struct ustr filename, unifilename;
- int len;
+ struct ustr *filename, *unifilename;
+ int len = 0;
- if (udf_build_ustr_exact(&unifilename, sname, flen))
+ filename = kmalloc(sizeof(struct ustr), GFP_NOFS);
+ if (!filename)
return 0;
+ unifilename = kmalloc(sizeof(struct ustr), GFP_NOFS);
+ if (!unifilename)
+ goto out1;
+
+ if (udf_build_ustr_exact(unifilename, sname, flen))
+ goto out2;
+
if (UDF_QUERY_FLAG(sb, UDF_FLAG_UTF8)) {
- if (!udf_CS0toUTF8(&filename, &unifilename)) {
+ if (!udf_CS0toUTF8(filename, unifilename)) {
udf_debug("Failed in udf_get_filename: sname = %s\n",
sname);
- return 0;
+ goto out2;
}
} else if (UDF_QUERY_FLAG(sb, UDF_FLAG_NLS_MAP)) {
- if (!udf_CS0toNLS(UDF_SB(sb)->s_nls_map, &filename,
- &unifilename)) {
+ if (!udf_CS0toNLS(UDF_SB(sb)->s_nls_map, filename,
+ unifilename)) {
udf_debug("Failed in udf_get_filename: sname = %s\n",
sname);
- return 0;
+ goto out2;
}
} else
- return 0;
-
- len = udf_translate_to_linux(dname, filename.u_name, filename.u_len,
- unifilename.u_name, unifilename.u_len);
- if (len)
- return len;
-
- return 0;
+ goto out2;
+
+ len = udf_translate_to_linux(dname, filename->u_name, filename->u_len,
+ unifilename->u_name, unifilename->u_len);
+out2:
+ kfree(unifilename);
+out1:
+ kfree(filename);
+ return len;
}
int udf_put_filename(struct super_block *sb, const uint8_t *sname,
--
1.5.6.4
On Sun, 16 Nov 2008 19:02:45 +0100
Marcin Slusarz <[email protected]> wrote:
> + filename = kmalloc(sizeof(struct ustr), GFP_NOFS);
I suspect that we could have used the superior GFP_KERNEL everywhere in
both these patches. But I'll let Jan worry about that ;)
On Tue 18-11-08 16:19:38, Andrew Morton wrote:
> On Sun, 16 Nov 2008 19:02:45 +0100
> Marcin Slusarz <[email protected]> wrote:
>
> > + filename = kmalloc(sizeof(struct ustr), GFP_NOFS);
>
> I suspect that we could have used the superior GFP_KERNEL everywhere in
> both these patches. But I'll let Jan worry about that ;)
Definitely not in the second case - that one is called from inside
readdir, lookup and symlink resolution code so that could lead to deadlocks
IMHO.
Regarding the first case in process_sequence, that is called only from
udf_fill_super(). So there it might be safe to use GFP_KERNEL but I'm not
quite sure either... So I'd leave GFP_NOFS there.
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Sun 16-11-08 19:02:45, Marcin Slusarz wrote:
> Allocate strings with kmalloc.
>
> Checkstack output:
> Before: udf_get_filename: 600
> After: udf_get_filename: 136
>
> Signed-off-by: Marcin Slusarz <[email protected]>
> Cc: Jan Kara <[email protected]>
Both patches look fine. Thanks Martin. I've merged them to my UDF tree.
Honza
> ---
> fs/udf/unicode.c | 41 +++++++++++++++++++++++++----------------
> 1 files changed, 25 insertions(+), 16 deletions(-)
>
> diff --git a/fs/udf/unicode.c b/fs/udf/unicode.c
> index 9fdf8c9..a3bbdbd 100644
> --- a/fs/udf/unicode.c
> +++ b/fs/udf/unicode.c
> @@ -324,34 +324,43 @@ try_again:
> int udf_get_filename(struct super_block *sb, uint8_t *sname, uint8_t *dname,
> int flen)
> {
> - struct ustr filename, unifilename;
> - int len;
> + struct ustr *filename, *unifilename;
> + int len = 0;
>
> - if (udf_build_ustr_exact(&unifilename, sname, flen))
> + filename = kmalloc(sizeof(struct ustr), GFP_NOFS);
> + if (!filename)
> return 0;
>
> + unifilename = kmalloc(sizeof(struct ustr), GFP_NOFS);
> + if (!unifilename)
> + goto out1;
> +
> + if (udf_build_ustr_exact(unifilename, sname, flen))
> + goto out2;
> +
> if (UDF_QUERY_FLAG(sb, UDF_FLAG_UTF8)) {
> - if (!udf_CS0toUTF8(&filename, &unifilename)) {
> + if (!udf_CS0toUTF8(filename, unifilename)) {
> udf_debug("Failed in udf_get_filename: sname = %s\n",
> sname);
> - return 0;
> + goto out2;
> }
> } else if (UDF_QUERY_FLAG(sb, UDF_FLAG_NLS_MAP)) {
> - if (!udf_CS0toNLS(UDF_SB(sb)->s_nls_map, &filename,
> - &unifilename)) {
> + if (!udf_CS0toNLS(UDF_SB(sb)->s_nls_map, filename,
> + unifilename)) {
> udf_debug("Failed in udf_get_filename: sname = %s\n",
> sname);
> - return 0;
> + goto out2;
> }
> } else
> - return 0;
> -
> - len = udf_translate_to_linux(dname, filename.u_name, filename.u_len,
> - unifilename.u_name, unifilename.u_len);
> - if (len)
> - return len;
> -
> - return 0;
> + goto out2;
> +
> + len = udf_translate_to_linux(dname, filename->u_name, filename->u_len,
> + unifilename->u_name, unifilename->u_len);
> +out2:
> + kfree(unifilename);
> +out1:
> + kfree(filename);
> + return len;
> }
>
> int udf_put_filename(struct super_block *sb, const uint8_t *sname,
> --
> 1.5.6.4
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Wed, 19 Nov 2008 16:26:22 +0100 Jan Kara <[email protected]> wrote:
> On Tue 18-11-08 16:19:38, Andrew Morton wrote:
> > On Sun, 16 Nov 2008 19:02:45 +0100
> > Marcin Slusarz <[email protected]> wrote:
> >
> > > + filename = kmalloc(sizeof(struct ustr), GFP_NOFS);
> >
> > I suspect that we could have used the superior GFP_KERNEL everywhere in
> > both these patches. But I'll let Jan worry about that ;)
> Definitely not in the second case - that one is called from inside
> readdir, lookup and symlink resolution code so that could lead to deadlocks
> IMHO.
> Regarding the first case in process_sequence, that is called only from
> udf_fill_super(). So there it might be safe to use GFP_KERNEL but I'm not
> quite sure either... So I'd leave GFP_NOFS there.
>
The reason for using GFP_NOFS is to prevent deadlocks when direct
memory reclaim reenters the filesystem code. But I don't think there's
ever a case when direct reclaim would enter the namespace part of a
filesystem - it is only expected to touch the pagecache (ie: data)
operations: writepage(), block allocator, etc.
On Wed 19-11-08 09:35:15, Andrew Morton wrote:
> On Wed, 19 Nov 2008 16:26:22 +0100 Jan Kara <[email protected]> wrote:
>
> > On Tue 18-11-08 16:19:38, Andrew Morton wrote:
> > > On Sun, 16 Nov 2008 19:02:45 +0100
> > > Marcin Slusarz <[email protected]> wrote:
> > >
> > > > + filename = kmalloc(sizeof(struct ustr), GFP_NOFS);
> > >
> > > I suspect that we could have used the superior GFP_KERNEL everywhere in
> > > both these patches. But I'll let Jan worry about that ;)
> > Definitely not in the second case - that one is called from inside
> > readdir, lookup and symlink resolution code so that could lead to deadlocks
> > IMHO.
> > Regarding the first case in process_sequence, that is called only from
> > udf_fill_super(). So there it might be safe to use GFP_KERNEL but I'm not
> > quite sure either... So I'd leave GFP_NOFS there.
> >
>
> The reason for using GFP_NOFS is to prevent deadlocks when direct
> memory reclaim reenters the filesystem code. But I don't think there's
> ever a case when direct reclaim would enter the namespace part of a
> filesystem - it is only expected to touch the pagecache (ie: data)
> operations: writepage(), block allocator, etc.
Hmm, but I see for example:
static int shrink_icache_memory(int nr, gfp_t gfp_mask)
{
if (nr) {
/*
* Nasty deadlock avoidance. We may hold various FS locks,
* and we don't want to recurse into the FS that called us
* in clear_inode() and friends..
*/
if (!(gfp_mask & __GFP_FS))
return -1;
prune_icache(nr);
}
return (inodes_stat.nr_unused / 100) * sysctl_vfs_cache_pressure;
}
So it seems that with GFP_KERNEL, prune_icache() can be called as well
(and similarly prune_dcache()) and that could in theory block on other
locks, couldn't it?
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Wed, 19 Nov 2008 22:01:23 +0100
Jan Kara <[email protected]> wrote:
> On Wed 19-11-08 09:35:15, Andrew Morton wrote:
> > On Wed, 19 Nov 2008 16:26:22 +0100 Jan Kara <[email protected]> wrote:
> >
> > > On Tue 18-11-08 16:19:38, Andrew Morton wrote:
> > > > On Sun, 16 Nov 2008 19:02:45 +0100
> > > > Marcin Slusarz <[email protected]> wrote:
> > > >
> > > > > + filename = kmalloc(sizeof(struct ustr), GFP_NOFS);
> > > >
> > > > I suspect that we could have used the superior GFP_KERNEL everywhere in
> > > > both these patches. But I'll let Jan worry about that ;)
> > > Definitely not in the second case - that one is called from inside
> > > readdir, lookup and symlink resolution code so that could lead to deadlocks
> > > IMHO.
> > > Regarding the first case in process_sequence, that is called only from
> > > udf_fill_super(). So there it might be safe to use GFP_KERNEL but I'm not
> > > quite sure either... So I'd leave GFP_NOFS there.
> > >
> >
> > The reason for using GFP_NOFS is to prevent deadlocks when direct
> > memory reclaim reenters the filesystem code. But I don't think there's
> > ever a case when direct reclaim would enter the namespace part of a
> > filesystem - it is only expected to touch the pagecache (ie: data)
> > operations: writepage(), block allocator, etc.
> Hmm, but I see for example:
> static int shrink_icache_memory(int nr, gfp_t gfp_mask)
> {
> if (nr) {
> /*
> * Nasty deadlock avoidance. We may hold various FS locks,
> * and we don't want to recurse into the FS that called us
> * in clear_inode() and friends..
> */
> if (!(gfp_mask & __GFP_FS))
> return -1;
> prune_icache(nr);
> }
> return (inodes_stat.nr_unused / 100) * sysctl_vfs_cache_pressure;
> }
> So it seems that with GFP_KERNEL, prune_icache() can be called as well
> (and similarly prune_dcache()) and that could in theory block on other
> locks, couldn't it?
>
hm, yeah, OK, true.
iirc this only applies to weird filessytems which do complex things
(ie: take locks) in their destroy_inode/clear_inode/etc handlers.
udf_clear_inode() looks pretty complex.