[Linux-cachefs] [PATCH] ceph: Add FScache support

[Milosz Tanski]

David,

On a somewhat related node. The header definition of
fscache_maybe_release_page doesn't seam quite correct. The comment
states it should return true if the page can be freed ... yet when
there's not a cookie or PG_fscache is not set it returns false. This
doesn't seam right, in fact the NFS code wraps this bit in a second
PageFsCache check to make sure to return true.

661 /**
662  * fscache_maybe_release_page - Consider releasing a page,
cancelling a store
663  * @cookie: The cookie representing the cache object
664  * @page: The netfs page that is being cached.
665  * @gfp: The gfp flags passed to releasepage()
666  *
667  * Consider releasing a page for the vmscan algorithm, on behalf
of the netfs's
668  * releasepage() call.  A storage request on the page may cancelled if it is
669  * not currently being processed.
670  *
671  * The function returns true if the page no longer has a storage
request on it,
672  * and false if a storage request is left in place.  If true is
returned, the
673  * page will have been passed to fscache_uncache_page().  If false
is returned
674  * the page cannot be freed yet.
675  */
676 static inline
677 bool fscache_maybe_release_page(struct fscache_cookie *cookie,
678                                 struct page *page,
679                                 gfp_t gfp)
680 {
681         if (fscache_cookie_valid(cookie) && PageFsCache(page))
682                 return __fscache_maybe_release_page(cookie, page, gfp);
683         return false;
684 }

On Tue, Jul 9, 2013 at 8:46 AM, Milosz Tanski <milosz at adfin.com> wrote:
> On Tue, Jul 9, 2013 at 6:33 AM, David Howells <dhowells at redhat.com> wrote:
>> Milosz Tanski <milosz at adfin.com> wrote:
>>
>>> It looks like both the cifs and NFS code do not bother with any
>>> locking around cifs_fscache_set_inode_cookie. Is there no concern over
>>> multiple open() calls racing to create the cookie in those
>>> filesystems?
>>
>> Yeah...  That's probably wrong.  AFS obviates the need for special locking by
>> doing it in afs_iget().
>
> I'm going to create a mutex around the enable cache / disable cache in
> the Ceph code since the spinlock is also right now.
>
>>
>> Hmmm...  I think I've just spotted what might be the cause of pages getting
>> marked PG_fscache whilst belonging to the allocator.
>>
>>         void nfs_fscache_set_inode_cookie(struct inode *inode, struct file *filp)
>>         {
>>                 if (NFS_FSCACHE(inode)) {
>>                         nfs_fscache_inode_lock(inode);
>>                         if ((filp->f_flags & O_ACCMODE) != O_RDONLY)
>>                                 nfs_fscache_disable_inode_cookie(inode);
>>                         else
>>                                 nfs_fscache_enable_inode_cookie(inode);
>>                         nfs_fscache_inode_unlock(inode);
>>                 }
>>         }
>>
>> can release the cookie whilst reads are in progress on it when an inode being
>> read suddenly changes to an inode being written.  We need some sort of
>> synchronisation on that there.
>
> So far my experience has been that synchronization has been the most
> tricky part of implementing fscache for Ceph. Things work great when
> there's a simple shell accessing data and break down when you're doing
> a HPC kind of workload.
>
>>
>> David