Use vmem_free() in dfl_free() and add dfl_alloc() #543
Conversation
|
@dweeezil I must have accidentally applied a stale version e5f9a9a when this was merged. LGTM. As for 9ac4097 see my question in openzfs/zfs#4529. |
| @@ -48,7 +48,7 @@ typedef struct dkioc_free_list_s { | |||
| } dkioc_free_list_t; | |||
|
|
|||
| static inline void dfl_free(dkioc_free_list_t *dfl) { | |||
| kmem_free(dfl, DFL_SZ(dfl->dfl_num_exts)); | |||
| vmem_free(dfl, DFL_SZ(dfl->dfl_num_exts)); | |||
There was a problem hiding this comment.
Shouldn't there be a dfl_alloc() counterpart to prevent this kind of issue. I see it's directly calling vmem_alloc() in the trim code.
There was a problem hiding this comment.
It would make perfect sense. I think the only reason we don't have one is because illumos doesn't have one. I do like the idea of adding one. We've already got the kmem/vmem difference from the upstream and I suppose a dfl_alloc() would make the intention more obvious for future porting efforts.
There was a problem hiding this comment.
Then let's add one. We're already forced to diverge slightly here, let's diverge in a way which can in theory be pushed back upstream.
This change was lost, somehow, in e5f9a9a. Since the arrays can be rather large, they need to be allocated with vmem_zalloc() via dfl_alloc() and freed with vmem_free() via dfl_free(). The new dfl_alloc() function should be used to allocate object of type dkioc_free_list_t in order that they're allocated from vmem.
The problem described in 2a5d574 also applies to XFS's file or inode fallocate method. When layered on XFS a warning will be emitted under CentOS7 when entering either the file or inode fallocate method with PF_FSTRANS already set. To avoid triggering this error PF_FSTRANS is cleared and then reset in vn_space().
dweeezil
changed the title
|
LGTM. My only concern would be if we need to leave PF_FSTRANS clear over |
|
The "disable PF_FSTRANS" portions LGTM. However, I'd like to make it clear that the issue stems from the portion of XFS which is writing pages to disk, in my bug report it just so happened that page write code was invoked from fallocate. Having patched the fallocate callsite doesn't mean such warnings won't occur in the future. In any case, I'm happy with this version being merged. |
|
Regarding the As to the commit comment, I mainly cloned the one from 2a5d574. I suppose some different wording could be in order. |
|
OK, then I'll merge them both as is. I can add additional context to the commit message to make it clear under exactly what context this can occur and link back to the issue. |
This change was lost, somehow, in e5f9a9a. Since the arrays can be rather large, they need to be allocated with vmem_zalloc() via dfl_alloc() and freed with vmem_free() via dfl_free(). The new dfl_alloc() function should be used to allocate object of type dkioc_free_list_t in order that they're allocated from vmem. Signed-off-by: Brian Behlendorf <[email protected]> Signed-off-by: Tim Chase <[email protected]> Signed-off-by: Nikolay Borisov <[email protected]> Closes openzfs#543
This change was lost, somehow, in e5f9a9a. Since the arrays can be
rather large, they need to be allocated with vmem_zalloc() and freed
with vmem_free().