by Minchan Kim

[permalink] [raw]

Subject: Re: [PATCH V2 0/3] drivers/staging: zcache: dynamic page cache/swap compression

On Wed, Feb 16, 2011 at 10:27 AM, Dan Magenheimer
<[email protected]> wrote:
>> -----Original Message-----
>> From: Matt [mailto:[email protected]]
>> Sent: Tuesday, February 15, 2011 5:12 PM
>> To: Minchan Kim
>> Cc: Dan Magenheimer; [email protected]; Chris Mason; linux-
>> [email protected]; [email protected]; [email protected]; linux-
>> [email protected]; Josef Bacik; Dan Rosenberg; Yan Zheng;
>> [email protected]; Li Zefan
>> Subject: Re: [PATCH V2 0/3] drivers/staging: zcache: dynamic page
>> cache/swap compression
>>
>> On Mon, Feb 14, 2011 at 4:35 AM, Minchan Kim <[email protected]>
>> > Just my guessing. I might be wrong.
>> >
>> > __cleancache_flush_inode calls cleancache_get_key with
>> cleancache_filekey.
>> > cleancache_file_key's size is just 6 * u32.
>> > cleancache_get_key calls btrfs_encode_fh with the key.
>> > but btrfs_encode_fh does typecasting the key to btrfs_fid which is
>> > bigger size than cleancache_filekey's one so it should not access
>> > fields beyond cleancache_get_key.
>> >
>> > I think some file systems use extend fid so in there, this problem
>> can
>> > happen. I don't know why we can't find it earlier. Maybe Dan and
>> > others test it for a long time.
>> >
>> > Am I missing something?
>> >
>> >
>> >
>> > --
>> > Kind regards,
>> > Minchan Kim
>> >
>>
>> reposting Minchan's message for reference to the btrfs mailing list
>> while also adding
>>
>> Li Zefan, Miao Xie, Yan Zheng, Dan Rosenberg and Josef Bacik to CC
>>
>> Regards
>>
>> Matt
>
> Hi Matt and Minchan --
>
> (BTRFS EXPERTS SEE *** BELOW)
>
> I definitely see a bug in cleancache_get_key in the monolithic
> zcache+cleancache+frontswap patch I posted on oss.oracle.com
> that is corrected in linux-next but I don't see how it could
> get provoked by btrfs.
>
> The bug is that, in cleancache_get_key, the return value of fhfn should
> be checked against 255. If the return value is 255, cleancache_get_key
> should return -1. This should disable cleancache for any filesystem
> where KEY_MAX is too large.
>
> But cleancache_get_key always calls fhfn with connectable == 0 and
> CLEANCACHE_KEY_MAX==6 should be greater than BTRFS_FID_SIZE_CONNECTABLE
> (which I think should be 5?). And the elements written into the
> typecast btrfs_fid should be only writing the first 5 32-bit words.

BTRFS_FID_SIZE_NON_CONNECTALBE is 5, not BTRFS_FID_SIZE_CONNECTABLE.
Anyway, you passed connectable with 0 so it should be only writing the
first 5 32-bit words as you said.
That's one I missed. ;-)

Thanks.
--
Kind regards,
Minchan Kim

2011-03-03 17:29:56

by Dan Magenheimer

[permalink] [raw]

Subject: RE: [PATCH V2 0/3] drivers/staging: zcache: dynamic page cache/swap compression

> > I definitely see a bug in cleancache_get_key in the monolithic
> > zcache+cleancache+frontswap patch I posted on oss.oracle.com
> > that is corrected in linux-next but I don't see how it could
> > get provoked by btrfs.
> >
> > The bug is that, in cleancache_get_key, the return value of fhfn
> should
> > be checked against 255. If the return value is 255,
> cleancache_get_key
> > should return -1. This should disable cleancache for any filesystem
> > where KEY_MAX is too large.
> >
> > But cleancache_get_key always calls fhfn with connectable == 0 and
> > CLEANCACHE_KEY_MAX==6 should be greater than
> BTRFS_FID_SIZE_CONNECTABLE
> > (which I think should be 5?). And the elements written into the
> > typecast btrfs_fid should be only writing the first 5 32-bit words.
>
> BTRFS_FID_SIZE_NON_CONNECTALBE is 5, not BTRFS_FID_SIZE_CONNECTABLE.
> Anyway, you passed connectable with 0 so it should be only writing the
> first 5 32-bit words as you said.
> That's one I missed. ;-)
>
> Thanks.
> --
> Kind regards,
> Minchan Kim

Sorry, I realized that I solved this with Matt offlist and never
posted the solution on-list, so for the archives:

This patch applies on top of the cleancache patch. It is really
a horrible hack but solving it correctly requires the interface
to encode_fh ops to change, which would require changes to many
filesystems, so best saved for a later time. If/when cleancache
gets merged, this patch will need to be applied on top of it
for btrfs to work properly when cleancache is enabled.

Basically, the problem is that, in all current filesystems,
obtaining the filehandle requires a dentry ONLY if connectable
is set. Otherwise, the dentry is only used to get the inode.
But cleancache_get_key only has an inode, and the alias list
of dentries associated with the inode may be empty. So
either the encode_fh interface would need to be changed
or, in this hack-y solution, a dentry is created temporarily
only for the purpose of dereferencing it.

Signed-off-by: Dan Magenheimer <[email protected]>

diff -Napur -X linux-2.6.37.1/Documentation/dontdiff linux-2.6.37.1/mm/cleancache.c linux-2.6.37.1-fix/mm/cleancache.c
--- linux-2.6.37.1/mm/cleancache.c 2011-02-25 11:38:47.000000000 -0800
+++ linux-2.6.37.1-fix/mm/cleancache.c 2011-02-25 08:53:46.000000000 -0800
@@ -78,15 +78,14 @@ static int cleancache_get_key(struct ino
int (*fhfn)(struct dentry *, __u32 *fh, int *, int);
int maxlen = CLEANCACHE_KEY_MAX;
struct super_block *sb = inode->i_sb;
- struct dentry *d;

key->u.ino = inode->i_ino;
if (sb->s_export_op != NULL) {
fhfn = sb->s_export_op->encode_fh;
if (fhfn) {
- d = list_first_entry(&inode->i_dentry,
- struct dentry, d_alias);
- (void)(*fhfn)(d, &key->u.fh[0], &maxlen, 0);
+ struct dentry d;
+ d.d_inode = inode;
+ (void)(*fhfn)(&d, &key->u.fh[0], &maxlen, 0);
if (maxlen > CLEANCACHE_KEY_MAX)
return -1;
}