Multipage Deallocator #19

DevonSchwartz · 2024-04-02T21:52:25Z

Changed data page deallocator to take a list of data pages and deallocate them without acquiring and releasing lock after each dealloc.

…public to private

…a page allocation

hayley-leblanc · 2024-04-04T13:49:44Z

fs/hayleyfs/balloc.rs

@@ -292,6 +294,7 @@ impl PageAllocator for Option<PerCpuPageAllocator> {

    fn dealloc_dir_page_list(&self, pages: &DirPageListWrapper<Clean, Free>) -> Result<()> {
        if let Some(allocator) = self {
+            let page_list = pages.get_page_list_cursor();


Remove this line, since we obtain the cursor for the list on the next line.

hayley-leblanc · 2024-04-04T13:51:49Z

fs/hayleyfs/balloc.rs

@@ -343,6 +346,83 @@ impl PerCpuPageAllocator {
            Ok(())
        }
    }
+
+
+    fn dealloc_multiple_page(&self, mut page_list : Cursor<'_, Box<LinkedPage>>) -> Result<()> {


It would be cleaner to move this code into dealloc_data_page_list so that we don't have to pass a Cursor type around.

hayley-leblanc · 2024-04-04T13:53:31Z

fs/hayleyfs/balloc.rs

+
+
+    fn dealloc_multiple_page(&self, mut page_list : Cursor<'_, Box<LinkedPage>>) -> Result<()> {
+        // hash map to store free list lock for every cpu


I'm a little confused by this comment -- it stores the free list for each CPU, not the lock, right?

You're absolutely right. I made sure to change the comment.

hayley-leblanc · 2024-04-04T13:54:02Z

fs/hayleyfs/balloc.rs

+            if let Some(page) = page {
+                pr_info!("Page: {}", page.get_page_no());
+
+                let cpu : usize = ((page.get_page_no() - self.start) / self.pages_per_cpu).try_into()?;


There might be a function defined elsewhere to do this calculation (if not, we should make one).

I'll go ahead and make a helper function to do this calculation.

hayley-leblanc · 2024-04-04T13:54:44Z

fs/hayleyfs/balloc.rs

+
+                let cpu : usize = ((page.get_page_no() - self.start) / self.pages_per_cpu).try_into()?;
+
+                if matches!(cpu_free_list_map.get(&cpu), None) {


Since we are only matching against one pattern, and it's the Option type, it would be more idiomatic/cleaner to do if cpu_free_list_map.get(&cpu).is_none().

hayley-leblanc · 2024-04-04T13:56:46Z

fs/hayleyfs/balloc.rs

+                }
+
+                // add cpu page to vector (vector is mutable)
+                let cpu_page_vec : &mut Vec<PageNum> = cpu_free_list_map.get_mut(&cpu).unwrap();


Use ? rather than unwrap here -- even though this should always succeed, it's safer to avoid using unwrap wherever possible, since the kernel will panic and crash if we call unwrap on an error.

When I change it to ?, I get the following compiler error:

"cannot use the ? operator in a method that returns `core::result::Result<(), kernel::Err

Update: I think I made the unwrap safe by calling is_some() on the option returned by the get_mut() function

hayley-leblanc · 2024-04-04T13:58:41Z

fs/hayleyfs/balloc.rs

+                    }
+                 };
+
+                // check that the page was not already present in the tree. CAUSING ERRORS DUE TO TYPE ANNOTATION ISSUES


What error is this running into?

I was able to resolve that error but forgot to delete the comment. I'll make sure to delete it.

… in dealloc_data_page_list and deallocating in PerCpuPageAllocator

DevonSchwartz · 2024-04-09T03:07:00Z

Applied changes. The deallocator has been tested with 1 CPU, but I am struggling to create a scenario where pages are allocated on multiple CPUs.

…tements

DevonSchwartz · 2024-04-12T22:21:40Z

Confirmed that pages removed from all cpus and removed prinfo statements.

hayley-leblanc · 2024-04-18T17:47:57Z

fs/hayleyfs/balloc.rs

+                    if cpu_free_list_map.get(&cpu).is_none() {
+                        cpu_free_list_map.try_insert(cpu, Vec::new())?; 
+                    }
+
+                    // add cpu page to vector (vector is mutable)
+                    let cpu_page_vec_option : Option<&mut Vec<PageNum>> = cpu_free_list_map.get_mut(&cpu);


It would be cleaner (and potentially more efficient) to take out the if statement that checks if cpu is in cpu_free_list_map; instead, you can obtain cpu_page_vec_option as you currently do, and then rather than returning an error if cpu_page_vec_option is None below, create a new vector with the page number and insert it into the map.

Also -- the convention here would be to just name the vector cpu_page_vec; it doesn't have to reflect the fact that it's an Option.

hayley-leblanc · 2024-04-18T17:51:53Z

fs/hayleyfs/balloc.rs

+                    if cpu_page_vec_option.is_some() {
+                        let cpu_page_vec : &mut Vec<PageNum> = cpu_page_vec_option.unwrap(); // safe unwrap
+                        cpu_page_vec.try_push(page.get_page_no())?;
+                    } else {
+                        pr_info!("CPU not added to RBTree\n");
+                        return Err(EINVAL); 
+                    }


Another Rust idiom -- you can structure this code as follows:

if let Some(cpu_page_vec) = cpu_page_vec_option() { cpu_page_vec.try_push(page.get_page_no())?; } else { . . . }

It's a bit more concise. If you rename cpu_page_vec_option to cpu_page_vec, Rust will also let you write the if condition like this:

if let Some(cpu_page_vec) = cpu_page _vec

Within the if case, cpu_page_vec will refer to the vector itself; outside of that, the variable name will still refer to the Option<Vec<>>.

hayley-leblanc · 2024-04-18T17:52:53Z

fs/hayleyfs/balloc.rs

+
+    fn dealloc_multiple_page(&self, cpu_free_list_map : RBTree<usize, Vec<PageNum>>) -> Result<()> {
+        // add pages to corresponding free list in ascending cpu # order
+        for (cpu, page_nos) in cpu_free_list_map.iter() {


Just to be sure -- have you confirmed/checked that this always iterates over the CPU keys in ascending order?

I've confirmed through dmesg that this iterates over CPU keys in ascending order (I think the RB tree default is ascending), but I don't know how to formally test it, so I'll remove the comment.

hayley-leblanc · 2024-04-18T17:54:38Z

fs/hayleyfs/balloc.rs

+    fn pageno2cpuid(&self, page_no : PageNum) -> Result<usize> {
+        let cpu: usize = ((page_no - self.start) / self.pages_per_cpu).try_into()?;
+        Ok(cpu)
+    }


Is there not already a function for this?

I haven't found one that does this in balloc.rs. I noticed in dealloc_page the same formula is used to calculate the cpu.

hayley-leblanc · 2024-04-18T17:58:01Z

reconfig.sh

+#!/bin/bash
+sudo umount /dev/pmem0
+sudo rmmod hayleyfs
+make LLVM=-14 fs/hayleyfs/hayleyfs.ko
+sudo insmod fs/hayleyfs/hayleyfs.ko
+sudo mount -o init -t hayleyfs /dev/pmem0 /mnt/pmem
+touch test.txt
+echo "Hello World" > test.txt
+sudo mv test.txt /mnt/pmem
+rm test.txt
+cd /mnt/pmem
+pwd


Have you written any tests beyond this? I think it would be good to add (in a tests/ directory at the top level) more cases, e.g. larger files, checking that everything works properly when there are multiple files in the system, etc.

I added two tests - one testing a large file and one testing a series of small files.

hayley-leblanc

The multi-page deallocate in SquirrelFS looks good! I've left a few suggestions/comments on the tests.

hayley-leblanc · 2024-09-03T13:52:32Z

evaluation/tests/large_remove.c

+long int enlarge_file(char *path, long int size) {
+    int fd = open(path, O_CREAT | O_RDWR); 
+    assert (fd > -1); 
+    lseek(fd, size, SEEK_SET); 


~~Does this actually increase the file's size? According to lseek documentation (https://man7.org/linux/man-pages/man2/lseek.2.html),~~

lseek() allows the file offset to be set beyond the end of the file (but this does not change the size of the file).

Edit: I see, the increase comes from the fputc writing a character at offset size. I would still suggest switching to one of the following approaches.

A more standard way of increasing the file's size (and one that would be sure to exercise page allocation code paths) would be to use the write system call to write a specified amount of data to the file: https://man7.org/linux/man-pages/man2/write.2.html, or the truncate syscall to set its size to a specified value. It doesn't actually matter what is written, so you could allocate a buffer but not update/set any of its bytes and just write whatever is already in it to the file. You can also do this with a FILE* obtained from fopen/fdopen using a function like fprintf, but the write system call may be easier because I think fprintf requires you to provide bytes to write as a string.

Great suggestion. I added a write call and it was much cleaner.

hayley-leblanc · 2024-09-03T13:59:30Z

evaluation/tests/large_remove.c

+    fputc('\0', fp);
+    fclose(fp);
+    close(fd);
+    fd = open(path, O_CREAT | O_RDWR); 


Why close and reopen the file?

You are write. I over complicated the code with lseek and changed to a write call.

hayley-leblanc · 2024-09-03T14:13:34Z

evaluation/tests/large_remove.c

+        long int new_size = enlarge_file(path, multiple * PAGESZ); 
+        used_all_pages = prev_size == new_size;
+        multiple = used_all_pages ? multiple : multiple + 1; 
+        prev_size = new_size; 


I'm not sure I understand exactly what this loop is doing; why increase multiple in this way?

The loop was intended to extend the file to use the maximum amount of pages before we run out, but that was overcomplicated. I now borrow the code from large_write and allocate 200000 pages instead.

hayley-leblanc · 2024-09-03T14:18:26Z

evaluation/tests/large_remove.c

+        prev_size = new_size; 
+    }
+    // a new file with one less page should be able to be allocated after removal 
+    assert(prev_size > 0);


A stronger check would be to assert that the file you created is the expected size.

hayley-leblanc · 2024-09-03T14:19:15Z

evaluation/tests/remove_multiple_files.c

+    for (int i = 0; i < num_files; i ++) {
+        sprintf(filename, "/mnt/pmem/%d", i);
+        int fd = open(filename, O_CREAT | O_RDWR);
+        lseek(fd, PAGESZ * 2, SEEK_SET); 
+        FILE *fp = fdopen(fd, "w");
+        assert(fp); 
+        putc('\0', fp);
+        fclose(fp);
+        close(fd);
+    }


Same comment as the other test w.r.t. increasing file size.

hayley-leblanc · 2024-09-03T14:19:54Z

evaluation/tests/large_remove.c

+
+    assert(statvfs("/mnt/pmem", &stat) == 0);
+    unsigned long pages_end = stat.f_bfree;
+    assert(pages_start == pages_end);


Another useful sanity check would be to ensure that between creating and deleting the big file, the number of pages in use has changed.

Devon added 4 commits February 10, 2024 14:32

Created multiple page deallocator

c42d438

Resolved issue of missing dependencies added when repo switched from …

1ba7094

…public to private

Added dealloc call to dir_page_list

2988a1a

Removed multiple page dealloc for directories and fixed issues in dat…

7e319f9

…a page allocation

hayley-leblanc reviewed Apr 4, 2024

View reviewed changes

Devon added 2 commits April 8, 2024 14:46

Resolved pull request issues. Biggest change was constructing RB tree…

b330656

… in dealloc_data_page_list and deallocating in PerCpuPageAllocator

Fixed unsafe wrap issue. New deallocate still untested

b60af97

Confirmed that pages are removed from all cpus and removed prinfo sta…

4148bf8

…tements

hayley-leblanc reviewed Apr 18, 2024

View reviewed changes

Devon added 7 commits August 19, 2024 14:17

Merge branch 'main' into page_alloc_changes

d4d2ece

balloc: refactored extraneous code

ef92b91

balloc: remove uneccessary comments

91eb603

remove reconfig

23cbbc8

tests: add a test for deallocating pages of large file

2cb4bff

tests: refactor large remove to check before and after free pages

29ae1d2

tests: tests creating and removing multiple files

85a99f9

DevonSchwartz requested a review from hayley-leblanc August 22, 2024 02:59

hayley-leblanc reviewed Sep 3, 2024

View reviewed changes

hayley-leblanc added 2 commits October 7, 2024 21:55

Added checks for pages in use

c10bb2d

use write instead of lseek to write to file

b0bb029

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multipage Deallocator #19

Multipage Deallocator #19

DevonSchwartz commented Apr 2, 2024

hayley-leblanc Apr 4, 2024

hayley-leblanc Apr 4, 2024

hayley-leblanc Apr 4, 2024

DevonSchwartz Apr 8, 2024

hayley-leblanc Apr 4, 2024

DevonSchwartz Apr 8, 2024

hayley-leblanc Apr 4, 2024

hayley-leblanc Apr 4, 2024

DevonSchwartz Apr 8, 2024 •

edited

Loading

hayley-leblanc Apr 4, 2024

DevonSchwartz Apr 8, 2024

DevonSchwartz commented Apr 9, 2024

DevonSchwartz commented Apr 12, 2024

hayley-leblanc Apr 18, 2024

hayley-leblanc Apr 18, 2024

hayley-leblanc Apr 18, 2024

DevonSchwartz Aug 21, 2024

hayley-leblanc Apr 18, 2024

DevonSchwartz Aug 21, 2024

hayley-leblanc Apr 18, 2024

DevonSchwartz Aug 22, 2024

hayley-leblanc left a comment

hayley-leblanc Sep 3, 2024

DevonSchwartz Oct 8, 2024

hayley-leblanc Sep 3, 2024

DevonSchwartz Oct 8, 2024

hayley-leblanc Sep 3, 2024

DevonSchwartz Oct 8, 2024

hayley-leblanc Sep 3, 2024

hayley-leblanc Sep 3, 2024

hayley-leblanc Sep 3, 2024



		fn dealloc_multiple_page(&self, mut page_list : Cursor<'_, Box<LinkedPage>>) -> Result<()> {
		// hash map to store free list lock for every cpu


		let cpu : usize = ((page.get_page_no() - self.start) / self.pages_per_cpu).try_into()?;

		if matches!(cpu_free_list_map.get(&cpu), None) {

Multipage Deallocator #19

Are you sure you want to change the base?

Multipage Deallocator #19

Conversation

DevonSchwartz commented Apr 2, 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

DevonSchwartz Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DevonSchwartz commented Apr 9, 2024

DevonSchwartz commented Apr 12, 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

hayley-leblanc left a 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

DevonSchwartz Apr 8, 2024 •

edited

Loading