Since we didn't override .can_trim or .can_zero, if the plugin supports that and we end up advertising it to the client, then the client trying to take advantage of them would end up trimming or zeroing at the wrong offsets in the plugin, probably corrupting the image. (Thankfully not an issue for nbdkit -r when you're just reading the file system, since nbdkit doesn't advertise trim or zero on read-only connections). If we don't override .can_flush, we are at the mercy of the plugin for whether the client will know that it can stabilize things into the ext2 filesystem (regardless of whether we can then further stabilize that filesystem back to the underlying plugin storage). Finally, the docs for nbdkit-filter say we should not call next_ops->cache unless can_cache returned NBDKIT_CACHE_NATIVE, but we were checking for equality to 1 (NBDKIT_CACHE_EMULATE). Fixes: 86b8466d260 (filters: Add ext2 filter) --- Pushed now, but still posting this to document the issue. filters/ext2/ext2.c | 47 ++++++++++++++++++++++++++++++++++++++++----- filters/ext2/io.c | 11 ++++++----- 2 files changed, 48 insertions(+), 10 deletions(-) diff --git a/filters/ext2/ext2.c b/filters/ext2/ext2.c index d7efb00c..1151effd 100644 --- a/filters/ext2/ext2.c +++ b/filters/ext2/ext2.c @@ -319,6 +319,45 @@ ext2_can_multi_conn (struct nbdkit_next_ops *next_ops, void *nxdata, return 0; } +static int +ext2_can_flush (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle) +{ + /* Regardless of the underlying plugin, we handle flush at the level + * of the filesystem. However, we also need to cache the underlying + * plugin ability, since ext2 wants to flush the filesystem into + * permanent storage when possible. + */ + if (next_ops->can_flush (nxdata) == -1) + return -1; + return 1; +} + +/* XXX It seems as if we should be able to support trim and zero, if + * we could work out how those are implemented in the ext2fs API which + * is very obscure. + */ +static int +ext2_can_zero (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle) +{ + /* For now, tell nbdkit to call .pwrite instead of any optimization. + * However, we also want to cache the underlying plugin support. + */ + if (next_ops->can_zero (nxdata) == -1) + return -1; + return NBDKIT_ZERO_EMULATE; +} + +static int +ext2_can_trim (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle) +{ + /* For now, tell nbdkit to never call .trim. However, we also want + * to cache the underlying plugin support. + */ + if (next_ops->can_trim (nxdata) == -1) + return -1; + return 0; +} + /* It might be possible to relax this, but it's complicated. * * It's desirable for ‘nbdkit -r’ to behave the same way as @@ -464,11 +503,6 @@ ext2_flush (struct nbdkit_next_ops *next_ops, void *nxdata, return 0; } -/* XXX It seems as if we should be able to support trim and zero, if - * we could work out how those are implemented in the ext2fs API which - * is very obscure. - */ - static struct nbdkit_filter filter = { .name = "ext2", .longname = "nbdkit ext2 filter", @@ -485,6 +519,9 @@ static struct nbdkit_filter filter = { .can_fua = ext2_can_fua, .can_cache = ext2_can_cache, .can_multi_conn = ext2_can_multi_conn, + .can_zero = ext2_can_zero, + .can_trim = ext2_can_trim, + .can_flush = ext2_can_flush, .export_description = ext2_export_description, .get_size = ext2_get_size, .pread = ext2_pread, diff --git a/filters/ext2/io.c b/filters/ext2/io.c index bab069f4..32c50683 100644 --- a/filters/ext2/io.c +++ b/filters/ext2/io.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2020 Red Hat Inc. + * Copyright (C) 2020-2021 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -310,7 +310,7 @@ io_cache_readahead (io_channel channel, data = (struct io_private_data *)channel->private_data; EXT2_CHECK_MAGIC (data, EXT2_ET_MAGIC_NBDKIT_IO_CHANNEL); - if (data->next_ops->can_cache (data->nxdata) == 1) { + if (data->next_ops->can_cache (data->nxdata) == NBDKIT_CACHE_NATIVE) { /* TODO is 32-bit overflow ever likely to be a problem? */ if (data->next_ops->cache (data->nxdata, (ext2_loff_t)count * channel->block_size, @@ -362,8 +362,9 @@ io_flush (io_channel channel) data = (struct io_private_data *) channel->private_data; EXT2_CHECK_MAGIC (data, EXT2_ET_MAGIC_NBDKIT_IO_CHANNEL); - if (data->next_ops->flush (data->nxdata, 0, &errno) == -1) - return errno; + if (data->next_ops->can_flush (data->nxdata) == 1) + if (data->next_ops->flush (data->nxdata, 0, &errno) == -1) + return errno; return retval; } @@ -432,7 +433,7 @@ io_zeroout (io_channel channel, unsigned long long block, data = (struct io_private_data *) channel->private_data; EXT2_CHECK_MAGIC (data, EXT2_ET_MAGIC_NBDKIT_IO_CHANNEL); - if (data->next_ops->can_zero (data->nxdata) == 1) { + if (data->next_ops->can_zero (data->nxdata) > NBDKIT_ZERO_NONE) { /* TODO is 32-bit overflow ever likely to be a problem? */ if (data->next_ops->zero (data->nxdata, (off_t)(count) * channel->block_size, -- 2.30.1