Punch hole #20

DevonSchwartz · 2024-04-17T15:33:02Z

Added punching hole capabilities for two cases:
When the length + offset is larger than the initial file size (similar to a truncate operation except we need to zero pages and keep the size the same)

Punching a hole but we are only zeroing bits, not deallocating

I configured read to output 0s if it encounters a deallocated page.

I will work on making punch_hole for the case where we deallocate and zero pages (a proper "hole).

I am also working on how to read sparse files in hayleyfs.

hayley-leblanc · 2024-04-22T14:14:10Z

fs/hayleyfs/h_file.rs

        _offset: i64,
        _len: i64,
    ) -> Result<u64> {


The convention in Rust is to prefix unused variables with _ (the compiler can even enforce this). These variables had a _ because the fallocate function was previously unimplemented so none of the arguments were used, but now that you have written an implementation, you should rename the arguments to remove the _.

hayley-leblanc · 2024-04-22T14:17:29Z

fs/hayleyfs/h_file.rs

@@ -134,7 +150,125 @@ impl file::Operations for FileOps {
        _offset: i64,
        _len: i64,
    ) -> Result<u64> {
-        Err(EINVAL)
+        let inode: &mut fs::INode = unsafe { &mut *_file.inode().cast() };


I think I meant to mention this in your deallocation PR and forgot; another Rust convention is to not explicitly specify the type (the : &mut fs::INode here, for example) unless you get a compiler error related to it. The Rust compiler can usually infer the type of each variable.

If you want type information to be displayed as a reminder to yourself, if you use VSCode, you can install the rust-analyzer extension and follow the instructions in the README to get it set up with our kernel. I think it still isn't great at labeling types that are defined outside of the file system, but it should label most variables with their types.

hayley-leblanc · 2024-04-22T14:17:48Z

fs/hayleyfs/h_file.rs

+        let pi = sbi.get_init_reg_inode_by_vfs_inode(inode.get_inner())?;
+        let initial_size: i64 = pi.get_size() as i64;
+
+        // Error checks beforehand


Is this meant to be a TODO?

hayley-leblanc · 2024-04-22T14:18:23Z

fs/hayleyfs/h_file.rs

+
+            if _offset > initial_size {
+                // cannot punch hole if offset is larger than size
+                return Ok(0); 


This should probably be an error (unless the documentation for fallocate says otherwise?)

When I do fallocate -p with a larger offset than size on my local machine, it doesn't error. It just keeps the file size the same.

hayley-leblanc · 2024-04-22T14:25:14Z

fs/hayleyfs/h_file.rs

+        let final_file_size : i64 = _len + _offset;  
+
+        let pi = sbi.get_init_reg_inode_by_vfs_inode(inode.get_inner())?;
+        let initial_size: i64 = pi.get_size() as i64;


Rust has two ways to convert between types -- you can either use as as you do here, or you can do an explicit conversion at runtime using (in this case) the following: pi.get_size().try_into(). .try_into() will, at runtime, try to convert the value into the desired type, potentially changing its actual bit-level representation if necessary. If this is not possible for some reason (e.g., you're starting with a u64 and converting to an i64 and the current is too big to fit into a signed 64 bit integer), .try_into() will return an error. When converting between numerical types, it's almost always better to use .try_into(), since it will ensure the resulting value is correctly converted and will return an error if the conversion cannot happen; as might just silently return something unexpected.

Using .try_into() is a case where sometimes you have to annotate the variable definition with its explicit type, since Rust needs to know exactly what type you are trying to end up with in order to compile.

hayley-leblanc · 2024-04-22T14:42:10Z

fs/hayleyfs/h_file.rs

+                    sbi.page_allocator.dealloc_data_page_list(&pages_to_unmap)?;
+
+                    // TODO: should this be done earlier or is it protected by locks?
+                    pi_info.remove_pages(&pages_to_unmap)?;


I think we are protected by a lock in VFS around the inode for this whole operation, so it should be safe to remove these pages here, but we should double check by examining the VFS code that invokes fallocate (we can also do that on Tuesday).

hayley-leblanc · 2024-04-22T14:43:01Z

fs/hayleyfs/h_file.rs

+                || (_offset as u64 == p_offset && (page_offset(end_hole)? == (p_offset))) {
+                pr_info!("Just Zeroing"); 
+
+                // only zero out bits because the hole does not deallocate from page_i to page_j start->end. No deallocation


Not sure I understand this case/comment -- eventually, the implementation will deallocate these pages, right? Is this case unfinished?

hayley-leblanc · 2024-04-22T14:43:16Z

fs/hayleyfs/h_file.rs

+                let pages_to_zero = DataPageListWrapper::get_data_page_list(pi.get_inode_info()?, bytes_to_zero as u64, _offset as u64)?;
+                match pages_to_zero {
+                    Ok(pages_to_zero) => {
+                        pages_to_zero.zero_pages(sbi, bytes_to_zero as u64, _offset as u64)?; 


Again, should save the result of zero_pages and make sure it is handled correctly.

hayley-leblanc · 2024-04-22T14:46:25Z

fs/hayleyfs/h_file.rs

+
+        if _mode & FALLOC_FLAG::FALLOC_FL_KEEP_SIZE as i32 == 1 {
+            // reset file size to original <-- truncate will add zeroed out pages
+            inode.i_size_write(initial_size.try_into()?);


This may have come from an earlier commit (i.e. not work on punch hole), but I'm not sure who is working on this one now, so I wanted to note that in this case, we shouldn't change the inode's size at all. There should be a separate function (other than hayleyfs_truncate) used to handle adding pages without changing the inode's size.

hayley-leblanc · 2024-04-22T14:50:30Z

fs/hayleyfs/h_file.rs

+                    count -= read;
+                    bytes_left_in_file -= read;
+                }
+                Err(_) => {


I need to think about/discuss it more, but I'm not sure if this is safe; we might need additional checks to confirm when an error returned by from_page_no means return zeros vs. when it should be handled as an actual error.

…ling division

…read

hayley-leblanc reviewed Apr 22, 2024

View reviewed changes

DevonSchwartz force-pushed the punch_hole branch 5 times, most recently from 6abbedb to e2c976e Compare August 26, 2024 03:55

DevonSchwartz added 5 commits August 25, 2024 22:57

fallocate: zeroed out partial start and end page

1078eef

truncate public

9ecf5cf

fallocate: fixed zero out end of partial file

4c68755

fallocate: fixed partial zero of last page in file through fixing cei…

c861cf4

…ling division

fallocate: deallocate pages but still seg fault on remove and cannot …

39dc359

…read

DevonSchwartz force-pushed the punch_hole branch from e2c976e to 39dc359 Compare August 26, 2024 03:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Punch hole #20

Punch hole #20

DevonSchwartz commented Apr 17, 2024

hayley-leblanc Apr 22, 2024

hayley-leblanc Apr 22, 2024

hayley-leblanc Apr 22, 2024

hayley-leblanc Apr 22, 2024

DevonSchwartz Aug 22, 2024

hayley-leblanc Apr 22, 2024

hayley-leblanc Apr 22, 2024

hayley-leblanc Apr 22, 2024

hayley-leblanc Apr 22, 2024

hayley-leblanc Apr 22, 2024

hayley-leblanc Apr 22, 2024

Punch hole #20

Are you sure you want to change the base?

Punch hole #20

Conversation

DevonSchwartz commented Apr 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment