Redfs-6.8 with dlm_lock opcode for fuse #5
Conversation
| area->offset = MIN(current_area_start_offset, lock_start_offset); | ||
| area->size = MAX(current_area_end_offset, lock_end_offset) - area->offset; | ||
| area->offset = min(current_area_start_offset, lock_start_offset); | ||
| area->size = max(current_area_end_offset, lock_end_offset) - area->offset; |
There was a problem hiding this comment.
Why backport? Linux master has min/max?
There was a problem hiding this comment.
I.e. I think this can just go as it is into the previous patch and can also be used for the upstream patch
| @@ -8,6 +8,8 @@ | |||
|
|
|||
| #include "fuse_i.h" | |||
|
|
|||
| #include "linux/list.h" | |||
| #include "linux/spinlock.h" | |||
There was a problem hiding this comment.
file.c already uses lists and uses spin-lock - uneeded includes.
| @@ -632,6 +632,7 @@ enum fuse_opcode { | |||
| FUSE_SYNCFS = 50, | |||
| FUSE_TMPFILE = 51, | |||
| FUSE_STATX = 52, | |||
| FUSE_DLM_LOCK = 53, | |||
There was a problem hiding this comment.
Right now this is only for write-back cache, right? FUSE_DLM_WB_LOCK?
| @@ -182,6 +199,7 @@ static void fuse_evict_inode(struct inode *inode) | |||
| if (S_ISREG(inode->i_mode) && !fuse_is_bad(inode)) { | |||
| WARN_ON(!list_empty(&fi->write_files)); | |||
| WARN_ON(!list_empty(&fi->queued_writes)); | |||
| fuse_forget_all_dlm_lock_areas(fi); | |||
There was a problem hiding this comment.
The condition on S_ISREG is giving me a bit headache. At least if there is nothing about write-back. Issue is that dir-inodes can and probably will have a page cache for readdir.
| struct dlm_locked_area *area, *tmp; | ||
| list_for_each_entry_safe(area, tmp, &fi->dlm_locked_areas, list) { | ||
| list_del(&area->list); | ||
| printk("free area %lld size: %ld\n", area->offset, area->size); |
There was a problem hiding this comment.
Debug printk should be removed. Btw, printk is outdated, recent way is pr_info. For debugging may change to pr_debug for now, which be enabled at run time.
| @@ -58,6 +58,7 @@ | |||
| EM( FUSE_SYNCFS, "FUSE_SYNCFS") \ | |||
| EM( FUSE_TMPFILE, "FUSE_TMPFILE") \ | |||
| EM( FUSE_STATX, "FUSE_STATX") \ | |||
| EM( FUSE_DLM_LOCK, "FUSE_DLM_LOCK") \ | |||
There was a problem hiding this comment.
FUSE_DLM_LOCK alignment doesn't look right. Spaces instead of tabs?
| @@ -142,6 +153,9 @@ struct fuse_inode { | |||
|
|
|||
| /* List of writepage requestst (pending or sent) */ | |||
| struct rb_root writepages; | |||
|
|
|||
| /** List of dlm locked areas we have sent lock requests for */ | |||
| struct list_head dlm_locked_areas; | |||
There was a problem hiding this comment.
Same here, dir-inodes also have a cache
|
|
||
| spin_lock(&fi->lock); | ||
| /* iterate through the areas */ | ||
| list_for_each_entry(area, &fi->dlm_locked_areas, list) { |
There was a problem hiding this comment.
I guess list will be too slow - this probably needs to be an xarray, rbtree or maple tree
| /* check if the locked areas are completely distinct, then we should continue */ | ||
| if (current_area_end_offset < lock_start_offset | ||
| || current_area_start_offset > lock_end_offset) | ||
| continue; |
There was a problem hiding this comment.
The problem with lists is that that random writes to large files will be really slow, I think
| } | ||
|
|
||
| /* create a new locked area */ | ||
| area = kmalloc(sizeof(struct dlm_locked_area), GFP_KERNEL); |
There was a problem hiding this comment.
kmalloc needs error checking
| return; | ||
|
|
||
| /* add the lock to the list of locked areas */ | ||
| if (outarg.locksize) { |
There was a problem hiding this comment.
Check missing if the server locked at least the requested size
Added DLM_LOCK opcode to fuse for signaling to fuse server that the kernel has mapped a page of a file in cache (only active when writeback cache is enabled)