Re: [systemd-devel] [PATCH 1/2] Revert patch 11689d2a which force the NOCOW attribute
On Sat, 11.04.15 14:29, Goffredo Baroncelli (kreij...@libero.it) wrote:
> Hi Lennart,
>
> On 2015-04-08 23:12, Lennart Poettering wrote:
> >> --- a/src/journal/journalctl.c
> >> > +++ b/src/journal/journalctl.c
> >> > @@ -1290,7 +1290,7 @@ static int setup_keys(void) {
> >> > size_t mpk_size, seed_size, state_size, i;
> >> > uint8_t *mpk, *seed, *state;
> >> > ssize_t l;
> >> > -int fd = -1, r;
> >> > +int fd = -1, r, attr = 0;
> >> > sd_id128_t machine, boot;
> >> > char *p = NULL, *k = NULL;
> >> > struct FSSHeader h;
> >> > @@ -1385,9 +1385,13 @@ static int setup_keys(void) {
> >> >
> >> > /* Enable secure remove, exclusion from dump, synchronous
> >> > * writing and in-place updating */
> >> > -r = chattr_fd(fd, true,
> >> > FS_SECRM_FL|FS_NODUMP_FL|FS_SYNC_FL|FS_NOCOW_FL);
> >> > -if (r < 0)
> >> > -log_warning_errno(errno, "Failed to set file
> >> > attributes: %m");
> >> > +if (ioctl(fd, FS_IOC_GETFLAGS, &attr) < 0)
> >> > +log_warning_errno(errno, "FS_IOC_GETFLAGS failed: %m");
> >> > +
> >> > +attr |= FS_SECRM_FL|FS_NODUMP_FL|FS_SYNC_FL|FS_NOCOW_FL;
> >> > +
> >> > +if (ioctl(fd, FS_IOC_SETFLAGS, &attr) < 0)
> >> > +log_warning_errno(errno, "FS_IOC_SETFLAGS failed: %m");
> > This is unrelated, and should not be reverted at all.
> >
> > Lennart
>
> Ok, this was a my fault to revert this chunk; anyway I would like to
> know which is the purpose of the FS_NOCOW_FL flag here. If I read
> the code, the file is few hundred bytes long, so in BTRFS this would
> be stored in the metadata chunk, and I am not sure if FS_NOCOW_FL is
> honored at all...
The FSS key file stores a generation key that is replaced by the next
generation in regular intervals. The old key should be flushed out at
that time, forgotten, and non-recoverable (because otherwise the
concept of FSS would be pointless...). COW file systems have the
tendency of being bad at "fogetting" things... Hence we set a variety
of bits to tell the filesystem to really get rid of the data when we
rewrite the file, including NOCOW, to avoid that the file is copied
when we write a newer key into...
Also, even if some file systems don't understand some of the bits, we
still should pass all that are appropriate to ensure that we make this
as good as we can even on (future) file systems whose behaviour we
don't know yet... (Let's not forget that the FS_NOCOW_FL is generic,
and could even be implemented by a new file system, maybe ZFS. It's
not called FS_BTRFS_NOCOW_FL after all..)
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 1/2] Revert patch 11689d2a which force the NOCOW attribute
Hi Lennart,
On 2015-04-08 23:12, Lennart Poettering wrote:
>> --- a/src/journal/journalctl.c
>> > +++ b/src/journal/journalctl.c
>> > @@ -1290,7 +1290,7 @@ static int setup_keys(void) {
>> > size_t mpk_size, seed_size, state_size, i;
>> > uint8_t *mpk, *seed, *state;
>> > ssize_t l;
>> > -int fd = -1, r;
>> > +int fd = -1, r, attr = 0;
>> > sd_id128_t machine, boot;
>> > char *p = NULL, *k = NULL;
>> > struct FSSHeader h;
>> > @@ -1385,9 +1385,13 @@ static int setup_keys(void) {
>> >
>> > /* Enable secure remove, exclusion from dump, synchronous
>> > * writing and in-place updating */
>> > -r = chattr_fd(fd, true,
>> > FS_SECRM_FL|FS_NODUMP_FL|FS_SYNC_FL|FS_NOCOW_FL);
>> > -if (r < 0)
>> > -log_warning_errno(errno, "Failed to set file attributes:
>> > %m");
>> > +if (ioctl(fd, FS_IOC_GETFLAGS, &attr) < 0)
>> > +log_warning_errno(errno, "FS_IOC_GETFLAGS failed: %m");
>> > +
>> > +attr |= FS_SECRM_FL|FS_NODUMP_FL|FS_SYNC_FL|FS_NOCOW_FL;
>> > +
>> > +if (ioctl(fd, FS_IOC_SETFLAGS, &attr) < 0)
>> > +log_warning_errno(errno, "FS_IOC_SETFLAGS failed: %m");
> This is unrelated, and should not be reverted at all.
>
> Lennart
Ok, this was a my fault to revert this chunk; anyway I would like to know which
is the purpose of the FS_NOCOW_FL flag here. If I read the code, the file is
few hundred bytes long, so in BTRFS this would be stored in the metadata chunk,
and I am not sure if FS_NOCOW_FL is honored at all...
--
gpg @keyserver.linux.it: Goffredo Baroncelli
Key fingerprint BBF5 1610 0B64 DAC6 5F7D 17B2 0EDA 9B37 8B82 E0B5
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 1/2] Revert patch 11689d2a which force the NOCOW attribute
On Sat, 21.03.15 12:56, Goffredo Baroncelli (kreij...@libero.it) wrote:
> From: Goffredo Baroncelli
>
> Revert patch 11689d2a, which force the NOCOW attribute for the journal files.
> This patch was introduced to allievate the perfomances problem that
> journald shows on the BTRFS filesystem.
>
> #include "btrfs-util.h"
> #include "journal-def.h"
> @@ -142,17 +141,8 @@ void journal_file_close(JournalFile *f) {
> if (f->mmap && f->fd >= 0)
> mmap_cache_close_fd(f->mmap, f->fd);
>
> -if (f->fd >= 0 && f->defrag_on_close) {
> -
> -/* Be friendly to btrfs: turn COW back on again now,
> - * and defragment the file. We won't write to the file
> - * ever again, hence remove all fragmentation, and
> - * reenable all the good bits COW usually provides
> - * (such as data checksumming). */
> -
> -(void) chattr_fd(f->fd, false, FS_NOCOW_FL);
> -(void) btrfs_defrag_fd(f->fd);
> -}
> +if (f->fd >= 0 && f->defrag_on_close)
> +btrfs_defrag_fd(f->fd);
I think this part should probably stay... I mean, it's actually
without effect since btrfs doesn't allow unsetting the COW flag right
now if the file is non-empty. But I still think we should do this, in
case it learns it one day. And after rotating the file there's really
no need to disable COW anymore...
>
> /* btrfs doesn't cope well with our write pattern and
> * fragments heavily. Let's defrag all files we rotate */
> -
> -(void) chattr_path(p, false, FS_NOCOW_FL);
> (void) btrfs_defrag(p);
This case is similar.
>
> log_warning("File %s corrupted or uncleanly shut down, renaming and
> replacing.", fname);
> diff --git a/src/journal/journalctl.c b/src/journal/journalctl.c
> index 55c7786..abb9d0c 100644
> --- a/src/journal/journalctl.c
> +++ b/src/journal/journalctl.c
> @@ -1290,7 +1290,7 @@ static int setup_keys(void) {
> size_t mpk_size, seed_size, state_size, i;
> uint8_t *mpk, *seed, *state;
> ssize_t l;
> -int fd = -1, r;
> +int fd = -1, r, attr = 0;
> sd_id128_t machine, boot;
> char *p = NULL, *k = NULL;
> struct FSSHeader h;
> @@ -1385,9 +1385,13 @@ static int setup_keys(void) {
>
> /* Enable secure remove, exclusion from dump, synchronous
> * writing and in-place updating */
> -r = chattr_fd(fd, true,
> FS_SECRM_FL|FS_NODUMP_FL|FS_SYNC_FL|FS_NOCOW_FL);
> -if (r < 0)
> -log_warning_errno(errno, "Failed to set file attributes:
> %m");
> +if (ioctl(fd, FS_IOC_GETFLAGS, &attr) < 0)
> +log_warning_errno(errno, "FS_IOC_GETFLAGS failed: %m");
> +
> +attr |= FS_SECRM_FL|FS_NODUMP_FL|FS_SYNC_FL|FS_NOCOW_FL;
> +
> +if (ioctl(fd, FS_IOC_SETFLAGS, &attr) < 0)
> +log_warning_errno(errno, "FS_IOC_SETFLAGS failed: %m");
This is unrelated, and should not be reverted at all.
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH 1/2] Revert patch 11689d2a which force the NOCOW attribute
From: Goffredo Baroncelli
Revert patch 11689d2a, which force the NOCOW attribute for the journal files.
This patch was introduced to allievate the perfomances problem that
journald shows on the BTRFS filesystem.
Because the NOCOW attribute is forced the user can't revert to
the old behavior. However NOCOW attribute disables the btrfs checksums, which
prevent BTRFS to rebuild a currupted file in an RAIDx filesystem.
To continue to set the NOCOW attribute, use the h|H command of systemd-tmpfile.
---
src/journal/journal-file.c | 28 ++--
src/journal/journalctl.c | 12
2 files changed, 10 insertions(+), 30 deletions(-)
diff --git a/src/journal/journal-file.c b/src/journal/journal-file.c
index 2845e05..9d56069 100644
--- a/src/journal/journal-file.c
+++ b/src/journal/journal-file.c
@@ -26,7 +26,6 @@
#include
#include
#include
-#include
#include "btrfs-util.h"
#include "journal-def.h"
@@ -142,17 +141,8 @@ void journal_file_close(JournalFile *f) {
if (f->mmap && f->fd >= 0)
mmap_cache_close_fd(f->mmap, f->fd);
-if (f->fd >= 0 && f->defrag_on_close) {
-
-/* Be friendly to btrfs: turn COW back on again now,
- * and defragment the file. We won't write to the file
- * ever again, hence remove all fragmentation, and
- * reenable all the good bits COW usually provides
- * (such as data checksumming). */
-
-(void) chattr_fd(f->fd, false, FS_NOCOW_FL);
-(void) btrfs_defrag_fd(f->fd);
-}
+if (f->fd >= 0 && f->defrag_on_close)
+btrfs_defrag_fd(f->fd);
safe_close(f->fd);
free(f->path);
@@ -2602,18 +2592,6 @@ int journal_file_open(
goto fail;
if (f->last_stat.st_size == 0 && f->writable) {
-
-/* Before we write anything, turn off COW logic. Given
- * our write pattern that is quite unfriendly to COW
- * file systems this should greatly improve
- * performance on COW file systems, such as btrfs, at
- * the expense of data integrity features (which
- * shouldn't be too bad, given that we do our own
- * checksumming). */
-r = chattr_fd(f->fd, true, FS_NOCOW_FL);
-if (r < 0)
-log_warning_errno(errno, "Failed to set file
attributes: %m");
-
/* Let's attach the creation time to the journal file,
* so that the vacuuming code knows the age of this
* file even if the file might end up corrupted one
@@ -2831,8 +2809,6 @@ int journal_file_open_reliably(
/* btrfs doesn't cope well with our write pattern and
* fragments heavily. Let's defrag all files we rotate */
-
-(void) chattr_path(p, false, FS_NOCOW_FL);
(void) btrfs_defrag(p);
log_warning("File %s corrupted or uncleanly shut down, renaming and
replacing.", fname);
diff --git a/src/journal/journalctl.c b/src/journal/journalctl.c
index 55c7786..abb9d0c 100644
--- a/src/journal/journalctl.c
+++ b/src/journal/journalctl.c
@@ -1290,7 +1290,7 @@ static int setup_keys(void) {
size_t mpk_size, seed_size, state_size, i;
uint8_t *mpk, *seed, *state;
ssize_t l;
-int fd = -1, r;
+int fd = -1, r, attr = 0;
sd_id128_t machine, boot;
char *p = NULL, *k = NULL;
struct FSSHeader h;
@@ -1385,9 +1385,13 @@ static int setup_keys(void) {
/* Enable secure remove, exclusion from dump, synchronous
* writing and in-place updating */
-r = chattr_fd(fd, true,
FS_SECRM_FL|FS_NODUMP_FL|FS_SYNC_FL|FS_NOCOW_FL);
-if (r < 0)
-log_warning_errno(errno, "Failed to set file attributes: %m");
+if (ioctl(fd, FS_IOC_GETFLAGS, &attr) < 0)
+log_warning_errno(errno, "FS_IOC_GETFLAGS failed: %m");
+
+attr |= FS_SECRM_FL|FS_NODUMP_FL|FS_SYNC_FL|FS_NOCOW_FL;
+
+if (ioctl(fd, FS_IOC_SETFLAGS, &attr) < 0)
+log_warning_errno(errno, "FS_IOC_SETFLAGS failed: %m");
zero(h);
memcpy(h.signature, "KSHHRHLP", 8);
--
2.1.4
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
