| Message ID | 306cbe63f85921ad40b00e3c7071be19cb6adf04.1375999914.git.milosz@adfin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hongyi Jia <jiayisuse@gmail.com> wrote: > +bool __fscache_check_consistency(struct fscache_cookie *cookie) > +{ > + struct fscache_object *object; > + > + if (cookie->def->type != FSCACHE_COOKIE_TYPE_DATAFILE) > + return false; > + > + if (hlist_empty(&cookie->backing_objects)) > + return false; > + > + object = hlist_entry(cookie->backing_objects.first, > + struct fscache_object, cookie_link); > + > + return object->cache->ops->check_consistency(object); > +} Hmmm... This isn't actually safe. You have to either: (1) hold cookie->lock whilst touching the object pointer when coming from the netfs side, or: (2) set up an operation to do this (as, say, __fscache_alloc_page() does). The problem is that you have nothing to defend against the object being withdrawn by the cache under you. David -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David, If the cache is withdrawn and we're starting anew I would consider that to okay. I would consider an empty page cache for a cookie to be consistent since there's nothing stale that I can read. Unless there's another synchronization issue that I'm missing in fscache. Thanks, - Milosz On Wed, Sep 4, 2013 at 12:24 PM, David Howells <dhowells@redhat.com> wrote: > Hongyi Jia <jiayisuse@gmail.com> wrote: > >> +bool __fscache_check_consistency(struct fscache_cookie *cookie) >> +{ >> + struct fscache_object *object; >> + >> + if (cookie->def->type != FSCACHE_COOKIE_TYPE_DATAFILE) >> + return false; >> + >> + if (hlist_empty(&cookie->backing_objects)) >> + return false; >> + >> + object = hlist_entry(cookie->backing_objects.first, >> + struct fscache_object, cookie_link); >> + >> + return object->cache->ops->check_consistency(object); >> +} > > Hmmm... This isn't actually safe. You have to either: > > (1) hold cookie->lock whilst touching the object pointer when coming from the > netfs side, or: > > (2) set up an operation to do this (as, say, __fscache_alloc_page() does). > > The problem is that you have nothing to defend against the object being > withdrawn by the cache under you. > > David -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Milosz Tanski <milosz@adfin.com> wrote: > If the cache is withdrawn and we're starting anew I would consider > that to okay. I would consider an empty page cache for a cookie to be > consistent since there's nothing stale that I can read. Unless there's > another synchronization issue that I'm missing in fscache. The problem is that the fscache_object struct may be deallocated whilst you're using it. David -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David, Is it as simple as stick a mutex at the top of the __fscache_check_consistency function before we try to find the object? This code should be called from a context that can sleep, in the Ceph code we call it from a delayed work queue (revalidate queue). -- Milosz On Wed, Sep 4, 2013 at 1:26 PM, David Howells <dhowells@redhat.com> wrote: > Milosz Tanski <milosz@adfin.com> wrote: > >> If the cache is withdrawn and we're starting anew I would consider >> that to okay. I would consider an empty page cache for a cookie to be >> consistent since there's nothing stale that I can read. Unless there's >> another synchronization issue that I'm missing in fscache. > > The problem is that the fscache_object struct may be deallocated whilst you're > using it. > > David -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Milosz Tanski <milosz@adfin.com> wrote: > Is it as simple as stick a mutex at the top of the > __fscache_check_consistency function before we try to find the object? You can lock a mutex in a function, but where are you actually going to place the mutex struct? And what other code is going to take it? To do this, you'd have to place the mutex struct in fscache_cookie. The problem is that you have to protect the pointer from fscache_cookie to fscache_object - so placing the mutex in fscache_object doesn't help. David -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 4, 2013 at 2:13 PM, David Howells <dhowells@redhat.com> wrote: > Milosz Tanski <milosz@adfin.com> wrote: > >> Is it as simple as stick a mutex at the top of the >> __fscache_check_consistency function before we try to find the object? > > You can lock a mutex in a function, but where are you actually going to place > the mutex struct? And what other code is going to take it? To do this, you'd > have to place the mutex struct in fscache_cookie. The problem is that you > have to protect the pointer from fscache_cookie to fscache_object - so placing > the mutex in fscache_object doesn't help. David, I meant lock cookie->lock inside of __fscache_check_consistency in the beginning of the function. I don't see the need to push this into the netfs code. - Milosz -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David, Is there anyway I can call you at (at your desk) during EST hours. I'd like to talk this part through since I think we're going a bit in circles. I'd like to get this fixed so we can submit the fscache for Ceph code for the upstream kernel in the merge window. Best, -- Milosz On Wed, Sep 4, 2013 at 3:41 PM, Milosz Tanski <milosz@adfin.com> wrote: > On Wed, Sep 4, 2013 at 2:13 PM, David Howells <dhowells@redhat.com> wrote: >> Milosz Tanski <milosz@adfin.com> wrote: >> >>> Is it as simple as stick a mutex at the top of the >>> __fscache_check_consistency function before we try to find the object? >> >> You can lock a mutex in a function, but where are you actually going to place >> the mutex struct? And what other code is going to take it? To do this, you'd >> have to place the mutex struct in fscache_cookie. The problem is that you >> have to protect the pointer from fscache_cookie to fscache_object - so placing >> the mutex in fscache_object doesn't help. > > David, > > I meant lock cookie->lock inside of __fscache_check_consistency in the > beginning of the function. I don't see the need to push this into the > netfs code. > > - Milosz -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/fscache/cookie.c b/fs/fscache/cookie.c index 0e91a3c..bfa1d3b 100644 --- a/fs/fscache/cookie.c +++ b/fs/fscache/cookie.c @@ -533,6 +533,28 @@ void __fscache_relinquish_cookie(struct fscache_cookie *cookie, int retire) EXPORT_SYMBOL(__fscache_relinquish_cookie); /* + * check the consistency between the netfs inode and the backing cache + * + * NOTE: it only serves no-index type + */ +bool __fscache_check_consistency(struct fscache_cookie *cookie) +{ + struct fscache_object *object; + + if (cookie->def->type != FSCACHE_COOKIE_TYPE_DATAFILE) + return false; + + if (hlist_empty(&cookie->backing_objects)) + return false; + + object = hlist_entry(cookie->backing_objects.first, + struct fscache_object, cookie_link); + + return object->cache->ops->check_consistency(object); +} +EXPORT_SYMBOL(__fscache_check_consistency); + +/* * destroy a cookie */ void __fscache_cookie_put(struct fscache_cookie *cookie) diff --git a/include/linux/fscache-cache.h b/include/linux/fscache-cache.h index a9ff9a3..5513342 100644 --- a/include/linux/fscache-cache.h +++ b/include/linux/fscache-cache.h @@ -302,6 +302,10 @@ struct fscache_cache_ops { /* dissociate a cache from all the pages it was backing */ void (*dissociate_pages)(struct fscache_cache *cache); + + /* check the consistency between the backing cache and the FS-Cache + * cookie */ + bool (*check_consistency)(struct fscache_object *object); }; /* diff --git a/include/linux/fscache.h b/include/linux/fscache.h index 7a08623..7a49e8f 100644 --- a/include/linux/fscache.h +++ b/include/linux/fscache.h @@ -183,6 +183,7 @@ extern struct fscache_cookie *__fscache_acquire_cookie( const struct fscache_cookie_def *, void *); extern void __fscache_relinquish_cookie(struct fscache_cookie *, int); +extern bool __fscache_check_consistency(struct fscache_cookie *); extern void __fscache_update_cookie(struct fscache_cookie *); extern int __fscache_attr_changed(struct fscache_cookie *); extern void __fscache_invalidate(struct fscache_cookie *); @@ -326,6 +327,22 @@ void fscache_relinquish_cookie(struct fscache_cookie *cookie, int retire) } /** + * fscache_check_consistency - Request that if the cache is updated + * @cookie: The cookie representing the cache object + * + * Request an consistency check from fscache, which resorts to backing + * cache. + */ +static inline +bool fscache_check_consistency(struct fscache_cookie *cookie) +{ + if (fscache_cookie_valid(cookie)) + return __fscache_check_consistency(cookie); + else + return false; +} + +/** * fscache_update_cookie - Request that a cache object be updated * @cookie: The cookie representing the cache object *