New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib/repo: Add min-free-space-percent option, default 3% #987

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@cgwalters
Member

cgwalters commented Jun 30, 2017

For ostree-as-host, we're the superuser, so we'll blow past
any reserved free space by default. While deltas have size
metadata, if one happens to do a loose fetch, we can fill
up the disk.

Another case is flatpak: the system helper has similar concerns
here as ostree-as-host, and for flatpak --user, we also
want to be nice and avoid filling up the user's quota.

Closes: ostreedev#962

@cgwalters

This comment has been minimized.

Show comment
Hide comment
@cgwalters

cgwalters Jun 30, 2017

Member

OK, this works pretty well in my testing, though I haven't added a unit or integration test yet. Debating a bit which to do.

Member

cgwalters commented Jun 30, 2017

OK, this works pretty well in my testing, though I haven't added a unit or integration test yet. Debating a bit which to do.

g_mutex_lock (&self->txn_stats_lock);
self->txn_blocksize = stvfsbuf.f_bsize;
/* Convert fragment to blocks to compute the total */
guint64 total_blocks = (stvfsbuf.f_frsize * stvfsbuf.f_blocks) / stvfsbuf.f_bsize;

This comment has been minimized.

@jlebon

jlebon Jun 30, 2017

Member

Doesn't this multiplication yield the total size of the filesystem in bytes already? statvfs(2) says:

fsblkcnt_t     f_blocks;   /* Size of fs in f_frsize units */

Writing a quick program to check this:

$ cat tmp.c                    
#include <sys/statvfs.h>                         
#include <stdio.h>      

int main(void)          
{                       
    struct statvfs stat;                         
    if (statvfs("/", &stat) < 0)                 
        return 1;       

    unsigned long long total_bytes = stat.f_blocks * stat.f_frsize;                                
    printf("total size in bytes: %llu\n", total_bytes);                                            
    printf("total size in GB: %llu\n", total_bytes >> 30);                                         

    return 0;           
}                       
$ ./tmp                                                                          
total size in bytes: 95133081600                 
total size in GB: 88    
$ df -hT /                                                                       
Filesystem              Type  Size  Used Avail Use% Mounted on                                     
/dev/mapper/fedora-root ext4   89G   63G   22G  75% /          

So then, we need to divide this by f_bsize to be made comparable to bfree, right?

@jlebon

jlebon Jun 30, 2017

Member

Doesn't this multiplication yield the total size of the filesystem in bytes already? statvfs(2) says:

fsblkcnt_t     f_blocks;   /* Size of fs in f_frsize units */

Writing a quick program to check this:

$ cat tmp.c                    
#include <sys/statvfs.h>                         
#include <stdio.h>      

int main(void)          
{                       
    struct statvfs stat;                         
    if (statvfs("/", &stat) < 0)                 
        return 1;       

    unsigned long long total_bytes = stat.f_blocks * stat.f_frsize;                                
    printf("total size in bytes: %llu\n", total_bytes);                                            
    printf("total size in GB: %llu\n", total_bytes >> 30);                                         

    return 0;           
}                       
$ ./tmp                                                                          
total size in bytes: 95133081600                 
total size in GB: 88    
$ df -hT /                                                                       
Filesystem              Type  Size  Used Avail Use% Mounted on                                     
/dev/mapper/fedora-root ext4   89G   63G   22G  75% /          

So then, we need to divide this by f_bsize to be made comparable to bfree, right?

This comment has been minimized.

@cgwalters

cgwalters Jun 30, 2017

Member

The first multiplication indeed yields size in bytes. But the reason we divide by the block size is that the intent is to do all calculations in terms of blocks, not bytes. That's how the filesystem works ultimately.

When we're looking at object sizes, we divide it by the block size (and add one to be conservative, though obviously in practice an object could land exactly on a block boundary).

@cgwalters

cgwalters Jun 30, 2017

Member

The first multiplication indeed yields size in bytes. But the reason we divide by the block size is that the intent is to do all calculations in terms of blocks, not bytes. That's how the filesystem works ultimately.

When we're looking at object sizes, we divide it by the block size (and add one to be conservative, though obviously in practice an object could land exactly on a block boundary).

This comment has been minimized.

@jlebon

jlebon Jun 30, 2017

Member

Oh goodness, I completely missed the / stvfsbuf.f_bsize; chunk of that line! (I really like the split view when looking at diffs, but this chops up lines much more when looking on my laptop).

@jlebon

jlebon Jun 30, 2017

Member

Oh goodness, I completely missed the / stvfsbuf.f_bsize; chunk of that line! (I really like the split view when looking at diffs, but this chops up lines much more when looking on my laptop).

@jlebon

This comment has been minimized.

Show comment
Hide comment
@jlebon

jlebon Jun 30, 2017

Member

OK, this works pretty well in my testing, though I haven't added a unit or integration test yet. Debating a bit which to do.

Hacky, but one way would be to add a test to the f25ah-insttest suite that uses loopback devices. The advantage is that we can easily try out different fs sizes and block sizes.

Member

jlebon commented Jun 30, 2017

OK, this works pretty well in my testing, though I haven't added a unit or integration test yet. Debating a bit which to do.

Hacky, but one way would be to add a test to the f25ah-insttest suite that uses loopback devices. The advantage is that we can easily try out different fs sizes and block sizes.

{
g_mutex_lock (&self->txn_stats_lock);
g_assert_cmpint (self->txn_blocksize, >, 0);
const fsblkcnt_t object_blocks = (size / self->txn_blocksize) + 1;

This comment has been minimized.

@cgwalters

cgwalters Jun 30, 2017

Member

(Here's where we convert objects into blocks)

@cgwalters

cgwalters Jun 30, 2017

Member

(Here's where we convert objects into blocks)

cgwalters added some commits Jun 29, 2017

lib/commit: Use provided length when doing writes
This is prep for storage space checks, where we look at free
space after parsing the metadata, before we write anything.

We did length-limited writes in the fd-based input path, but not for the
`GInputStream` path which in practice is used for HTTP pulls.
lib/repo: Add min-free-space-percent option, default 3%
For ostree-as-host, we're the superuser, so we'll blow past
any reserved free space by default.  While deltas have size
metadata, if one happens to do a loose fetch, we can fill
up the disk.

Another case is flatpak: the system helper has similar concerns
here as ostree-as-host, and for `flatpak --user`, we also
want to be nice and avoid filling up the user's quota.

Closes: #962
@cgwalters

This comment has been minimized.

Show comment
Hide comment
@cgwalters

cgwalters Jul 3, 2017

Member

🏄 Rebased and now with an installed 🏗 test

Member

cgwalters commented Jul 3, 2017

🏄 Rebased and now with an installed 🏗 test

@cgwalters cgwalters removed the reviewed label Jul 3, 2017

@jlebon

This comment has been minimized.

Show comment
Hide comment
@jlebon
Member

jlebon commented Jul 4, 2017

@rh-atomic-bot

This comment has been minimized.

Show comment
Hide comment
@rh-atomic-bot

rh-atomic-bot Jul 4, 2017

Collaborator

⌛️ Testing commit 4447145 with merge 1f5ce1a...

Collaborator

rh-atomic-bot commented Jul 4, 2017

⌛️ Testing commit 4447145 with merge 1f5ce1a...

rh-atomic-bot added a commit that referenced this pull request Jul 4, 2017

lib/repo: Add min-free-space-percent option, default 3%
For ostree-as-host, we're the superuser, so we'll blow past
any reserved free space by default.  While deltas have size
metadata, if one happens to do a loose fetch, we can fill
up the disk.

Another case is flatpak: the system helper has similar concerns
here as ostree-as-host, and for `flatpak --user`, we also
want to be nice and avoid filling up the user's quota.

Closes: #962

Closes: #987
Approved by: jlebon
@rh-atomic-bot

This comment has been minimized.

Show comment
Hide comment
@rh-atomic-bot

rh-atomic-bot Jul 4, 2017

Collaborator

☀️ Test successful - status-atomicjenkins
Approved by: jlebon
Pushing 1f5ce1a to master...

Collaborator

rh-atomic-bot commented Jul 4, 2017

☀️ Test successful - status-atomicjenkins
Approved by: jlebon
Pushing 1f5ce1a to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment