David,
I took your suggestions and updated my wip branch (on bitbucket) with
a handful of fixes except for the locking around registering the
cookie. I'm not sure what's the correct thing to do there.
On Tue, Jul 2, 2013 at 7:40 PM, David Howells <dhowells@xxxxxxxxxx> wrote:
>
> Okay, my analysis of the patch:
>
> Looking at your index structure, ceph has per-fsid indices under the top
> "ceph" index and then per-inode indices under those? Are fsids universally
> unique - or just for a given server/cell/whatever?
It's my understanding that's a guuid assigned to the cluster.
>
>> +#ifdef CONFIG_CEPH_FSCACHE
>> + if (PageFsCache(page))
>> + ceph_invalidate_fscache_page(inode, page);
>> +#endif
>
> The PageFsCache() test here should be folded into the header file. You
> actually have a redundant test:
>
> +static inline void ceph_invalidate_fscache_page(struct inode *inode,
> + struct page *page)
> +{
> + if (ceph_inode(inode)->fscache == NULL)
> + return;
> +
> + if (PageFsCache(page))
> + return __ceph_invalidate_fscache_page(inode, page);
> +}
>
>> +#ifdef CONFIG_CEPH_FSCACHE
>> + /* Can we release the page from the cache? */
>> + if (PageFsCache(page) && ceph_release_fscache_page(page, g) == 0)
>> + return 0;
>> +#endif
>
> The PageFsCache() test here is also redundant as fscache_maybe_release_page()
> also does it - though I acknowledge it does it after doing the "is this cookie
> valid" test. The reason I put that test first is that if CONFIG_FSCACHE=n
> then the "is this cookie valid" test just evaluates immediately to false at
> compile time.
>
>> +void ceph_fscache_inode_get_cookie(struct inode *inode);
>
> No such function?
Fixed.
>
>> +static inline void ceph_fsxache_async_uncache_inode(struct inode* inode)
>
> Misspelling?
Fixed.
>
>> +static inline int ceph_readpage_from_fscache(struct inode *inode,
>> + struct page *page)
>> +{
>> + if (ceph_inode(inode)->fscache == NULL)
>> + return -ENOBUFS;
>> +
>> + return __ceph_readpage_from_fscache(inode, page);
>> +}
>
> Looking at functions like this, if you wrap:
>
> ceph_inode(inode)->fscache
>
> as, say:
>
> struct fscache_cookie *ceph_inode_cookie(struct inode *inode)
> {
> #ifdef CONFIG_CEPH_FSCACHE
> return ceph_inode(inode)->fscache;
> #else
> return NULL;
> #endif
> }
>
> then you can get rid of a lot of cpp conditionals and just rely on gcc's
> optimiser (see what I did in include/linux/fscache.h). Note that anything in
> fs/ceph/cache.c wouldn't need to use this.
Per your suggestion I implemented this. This helped me to get rid of
some of the CONFIG_CEPH_FSCACHE checks (but not all).
>
>> static int ceph_init_file(struct inode *inode, struct file *file, int fmode)
>> ...
>> +#ifdef CONFIG_CEPH_FSCACHE
>> + spin_lock(&ci->i_ceph_lock);
>> + ceph_fscache_register_inode_cookie(fsc, ci);
>> + spin_unlock(&ci->i_ceph_lock);
>> +#endif
>
> Ummm... ceph_fscache_register_inode_cookie() calls fscache_acquire_cookie()
> which allocates GFP_KERNEL and grabs fscache_addremove_sem. You can't wrap
> this call in a spinlock. Do you use lockdep?
I took a look at the nfs code it seams like there isn't any kind of
locking in nfs_open around acquiring of the cookie. Then looking back
at Ceph code it seams like extensively locks in the open code. I'm not
sure if I have open worry about open races.
>
> David
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
