Re: Add operation vcache_rekey
On 26 Jun 2014, at 05:33, David Holland wrote: > On Wed, Jun 25, 2014 at 09:46:16AM +, Taylor R Campbell wrote: >> Also, I wonder whether any file systems will ever change the length of >> the key for a vnode. Probably not. > > As I recall from when I was looking at this a couple weeks ago (feels > like a minor eternity ago, but it can't be much longer than that) > ... yes, they might. David, they will. nfs/nfs_vnops.c::nfs_lookitup() will change the file handle in the "*npp != NULL" case. Here both the key and its length may change. I overlooked this one as it didn't update the rbtree. To make it worse the memory holding the file handle may get reallocated so it is impossible to do the rekey with one call. The attached diff adds int vcache_rekey_enter(struct mount *mp, struct vnode *vp, const void *old_key, size_t old_key_len, const void *new_key, size_t new_key_len) to prepare the key change. It will allocate a cache node for the new key and lock both the old and the new cache node. It is an error if the new node already exists. void vcache_rekey_exit(struct mount *mp, struct vnode *vp, const void *old_key, size_t old_key_len, const void *new_key, size_t new_key_len) to finish the key change. It will remove the old cache node and setup and unlock the new cache node. Comments, suggestions or objections? -- J. Hannken-Illjes - hann...@eis.cs.tu-bs.de - TU Braunschweig (Germany) vcache.diff Description: Binary data
Re: Add operation vcache_rekey
On 26 Jun 2014, at 05:33, David Holland wrote: > On Wed, Jun 25, 2014 at 09:46:16AM +, Taylor R Campbell wrote: >> Also, I wonder whether any file systems will ever change the length of >> the key for a vnode. Probably not. > > As I recall from when I was looking at this a couple weeks ago (feels > like a minor eternity ago, but it can't be much longer than that) > ... yes, they might. > > Please add the length in the appropriate places. My proposal HAS old and new key length. > also, as far as the const it seems like if replacing one key with > another requires unconsting something isn't right. The cache should > only be reading the key, and supplying a new one doesn't change that... How could we ATOMICALLY change a key when it is read-only aka. const? -- J. Hannken-Illjes - hann...@eis.cs.tu-bs.de - TU Braunschweig (Germany)
Re: Add operation vcache_rekey
On Wed, Jun 25, 2014 at 09:46:16AM +, Taylor R Campbell wrote: > Also, I wonder whether any file systems will ever change the length of > the key for a vnode. Probably not. As I recall from when I was looking at this a couple weeks ago (feels like a minor eternity ago, but it can't be much longer than that) ... yes, they might. Please add the length in the appropriate places. also, as far as the const it seems like if replacing one key with another requires unconsting something isn't right. The cache should only be reading the key, and supplying a new one doesn't change that... -- David A. Holland dholl...@netbsd.org
Re: Add operation vcache_rekey
Date: Wed, 25 Jun 2014 12:04:30 +0200
From: "J. Hannken-Illjes"
Using "memcpy(__UNCONST(node->vn_key.vk_key), new_key, new_key_len)"
works but I'm not sure if __UNCONST is the right way to go.
I think __UNCONST is the right way to go -- removing const from the
API itself is certainly not right. I would put the __UNCONST in the
initialization of temporary vcache keys used to look things up, e.g.
in vcache_get:
struct vcache_key {
struct mount *vk_mount;
void *vk_key;
size_t vk_key_len;
};
int
vcache_get(struct mount *mp, const void *key, size_t key_len,
struct vnode *vpp)
{
...
struct vcache_key vcache_key;
...
vcache_key.vk_mount = mp;
vcache_key.vk_key = __UNCONST(key);
vcache_key.vk_key_len = key_len;
}
Re: Add operation vcache_rekey
On 25 Jun 2014, at 11:46, Taylor R Campbell wrote: > Date: Wed, 25 Jun 2014 11:24:16 +0200 > From: "J. Hannken-Illjes" > > The attached diff adds a new vcache operation > > void > vcache_rekey(struct mount *mp, void *old_key, size_t old_key_len, > void *new_key, size_t new_key_len) > > to atomically change the key of a cached vnode. As the key is no longer > constant remove "const" from all key arguments. > > Comments or objections anyone? > > I assume you're doing this in preparation for replacing the msdosfs > ihash without breaking msdosfs_rename, yes? Yes. > I'm not sure all the const removal is sensible. For example, it's not > clear to me why vcache_get(mp, &ino, sizeof ino, &vp) should ever > modify ino. I think the only place in the API you really want to > remove const is in the loadvnode prototype: > > int xyzfs_loadvnode(struct mount *mp, struct vnode *vp, > const void *key, size_t key_len, void **new_key); > ^^ > And I think vcache_rekey should take const pointers too. (This may > require a little judicious __UNCONST in the implementation of the API > as an artefact of the dual use of struct vcache_key.) This is all because vk_key member cannot be "const" as it is the destination of memcpy(). This "const" removal then ripples up. Using "memcpy(__UNCONST(node->vn_key.vk_key), new_key, new_key_len)" works but I'm not sure if __UNCONST is the right way to go. > > But I may be missing something here. If I'm barking up the wrong tree > with this, could you show how you plan to use vcache_rekey with > msdosfs or whatever else needs it? > > Also, I wonder whether any file systems will ever change the length of > the key for a vnode. Probably not. -- J. Hannken-Illjes - hann...@eis.cs.tu-bs.de - TU Braunschweig (Germany)
Re: Add operation vcache_rekey
Date: Wed, 25 Jun 2014 11:24:16 +0200 From: "J. Hannken-Illjes" The attached diff adds a new vcache operation void vcache_rekey(struct mount *mp, void *old_key, size_t old_key_len, void *new_key, size_t new_key_len) to atomically change the key of a cached vnode. As the key is no longer constant remove "const" from all key arguments. Comments or objections anyone? I assume you're doing this in preparation for replacing the msdosfs ihash without breaking msdosfs_rename, yes? I'm not sure all the const removal is sensible. For example, it's not clear to me why vcache_get(mp, &ino, sizeof ino, &vp) should ever modify ino. I think the only place in the API you really want to remove const is in the loadvnode prototype: int xyzfs_loadvnode(struct mount *mp, struct vnode *vp, const void *key, size_t key_len, void **new_key); ^^ And I think vcache_rekey should take const pointers too. (This may require a little judicious __UNCONST in the implementation of the API as an artefact of the dual use of struct vcache_key.) But I may be missing something here. If I'm barking up the wrong tree with this, could you show how you plan to use vcache_rekey with msdosfs or whatever else needs it? Also, I wonder whether any file systems will ever change the length of the key for a vnode. Probably not.
