Currently, e4defrag creates a donor file _in the same directory_ of
the file being defrag'ed.
The problem with this is that the containing directory times are
changed because of the transient presence of the donor file. Any
thoughts as to moving the donor to a better location, mount-root, or
/lost+found? Or, ugh, a configurable location, if non-privileged
defrag is important?
+Reardon
On Fri, Aug 15, 2014 at 04:12:40PM -0400, TR Reardon wrote:
> Currently, e4defrag creates a donor file _in the same directory_ of
> the file being defrag'ed.
>
> The problem with this is that the containing directory times are
> changed because of the transient presence of the donor file. Any
> thoughts as to moving the donor to a better location, mount-root, or
> /lost+found? Or, ugh, a configurable location, if non-privileged
> defrag is important?
How about we just reset the directory time? (Or does utime() not work?)
--D
>
> +Reardon
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Fri, Aug 15, 2014 at 04:12:40PM -0400, TR Reardon wrote:
>> Currently, e4defrag creates a donor file _in the same directory_ of
>> the file being defrag'ed.
>>
>> The problem with this is that the containing directory times are
>> changed because of the transient presence of the donor file. Any
>> thoughts as to moving the donor to a better location, mount-root, or
>> /lost+found? Or, ugh, a configurable location, if non-privileged
>> defrag is important?
>
> How about we just reset the directory time? (Or does utime() not work?)
>
> --D
Er, yes, that's simpler. ?And a SIGINT handler just to keep things tidy?
+Reardon
-
The reason the donor file is created in the same directory as the source is to try and keep the block allocation policy consistent with the original inode.
You may not need a SIGINT handler, since the timestamp could be reset as soon as the file is created and unlinked.
It may also be possible to use O_TMPFILE on newer kernels to create the donor file to avoid any races?
Cheers, Andreas
> On Aug 15, 2014, at 22:45, TR Reardon <[email protected]> wrote:
>
>
>>> On Fri, Aug 15, 2014 at 04:12:40PM -0400, TR Reardon wrote:
>>> Currently, e4defrag creates a donor file _in the same directory_ of
>>> the file being defrag'ed.
>>>
>>> The problem with this is that the containing directory times are
>>> changed because of the transient presence of the donor file. Any
>>> thoughts as to moving the donor to a better location, mount-root, or
>>> /lost+found? Or, ugh, a configurable location, if non-privileged
>>> defrag is important?
>>
>> How about we just reset the directory time? (Or does utime() not work?)
>>
>> --D
>
> Er, yes, that's simpler. And a SIGINT handler just to keep things tidy?
>
> +Reardon
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Picking this back up. ?How would O_TMPFILE avoid races? ?It definitely avoids the unwanted mtime/atime update, but then the existing "<filename>.defrag" pseudo-lock file would no longer be available. ?How could you use O_TMPFILE and still avoid multiple defrag? ?If this isn't possible, then resetting the parent times on unlink(tmpfile), as you suggest, is the simplest way out of this.
+Reardon
> From: [email protected]
> Subject: Re: possible different donor file naming in e4defrag
> Date: Fri, 15 Aug 2014 23:04:21 +0200
> To: [email protected]
>
> The reason the donor file is created in the same directory as the source is to try and keep the block allocation policy consistent with the original inode.
>
> You may not need a SIGINT handler, since the timestamp could be reset as soon as the file is created and unlinked.
>
> It may also be possible to use O_TMPFILE on newer kernels to create the donor file to avoid any races?
>
> Cheers, Andreas
>
-
On Sep 11, 2014, at 1:48 PM, TR Reardon <[email protected]> wrote:
> Picking this back up. How would O_TMPFILE avoid races? It definitely avoids the unwanted mtime/atime update, but then the existing "<filename>.defrag" pseudo-lock file would no longer be available. How could you use O_TMPFILE and still avoid multiple defrag? If this isn't possible, then resetting the parent times on unlink(tmpfile), as you suggest, is the simplest way out of this.
Looking at this the opposite way - what are the chances that there
will be concurrent defrags on the same file? Basically no chance at
all. So long as it doesn't explode (the kernel would need to protect
against this anyway to avoid malicious apps), the worst case is that
there will be some extra defragmentation done in a very rare case.
Conversely, creating a temporary filename and then resetting the
parent directory timestamp is extra work for every file defragmented,
and is racy because e4defrag may "reset" the time to before the temp
file was created, but clobber a legitimate timestamp update in the
directory from some other concurrent update. That timestamp update
is always going to be racy, even if e4defrag tries to be careful.
Cheers, Andreas
>> From: [email protected]
>> Subject: Re: possible different donor file naming in e4defrag
>> Date: Fri, 15 Aug 2014 23:04:21 +0200
>> To: [email protected]
>>
>> The reason the donor file is created in the same directory as the source is to try and keep the block allocation policy consistent with the original inode.
>>
>> You may not need a SIGINT handler, since the timestamp could be reset as soon as the file is created and unlinked.
>>
>> It may also be possible to use O_TMPFILE on newer kernels to create the donor file to avoid any races?
>>
>> Cheers, Andreas
>>
>
>
Cheers, Andreas
> On Sep 11, 2014, at 1:48 PM, TR Reardon <[email protected]> wrote:
>> Picking this back up. How would O_TMPFILE avoid races? It definitely avoids the unwanted mtime/atime update, but then the existing "<filename>.defrag" pseudo-lock file would no longer be available. How could you use O_TMPFILE and still avoid multiple defrag? If this isn't possible, then resetting the parent times on unlink(tmpfile), as you suggest, is the simplest way out of this.
>
> Looking at this the opposite way - what are the chances that there
> will be concurrent defrags on the same file? Basically no chance at
> all. So long as it doesn't explode (the kernel would need to protect
> against this anyway to avoid malicious apps), the worst case is that
> there will be some extra defragmentation done in a very rare case.
>
> Conversely, creating a temporary filename and then resetting the
> parent directory timestamp is extra work for every file defragmented,
> and is racy because e4defrag may "reset" the time to before the temp
> file was created, but clobber a legitimate timestamp update in the
> directory from some other concurrent update. That timestamp update
> is always going to be racy, even if e4defrag tries to be careful.
>
> Cheers, Andreas
Thanks, well described.
So a simple attempt with O_TMPFILE first, then revert to original behavior, like below?
diff --git a/misc/e4defrag.c b/misc/e4defrag.c
index d0eac60..8001182 100644
--- a/misc/e4defrag.c
+++ b/misc/e4defrag.c
@@ -40,6 +40,7 @@
?#include <sys/stat.h>
?#include <sys/statfs.h>
?#include <sys/vfs.h>
+#include <libgen.h>
?
?/* A relatively new ioctl interface ... */
?#ifndef EXT4_IOC_MOVE_EXT
@@ -1526,31 +1527,36 @@ static int file_defrag(const char *file, const struct stat64 *buf,
?
? /* Create donor inode */
? memset(tmp_inode_name, 0, PATH_MAX + 8);
- sprintf(tmp_inode_name, "%.*s.defrag",
- (int)strnlen(file, PATH_MAX), file);
- donor_fd = open64(tmp_inode_name, O_WRONLY | O_CREAT | O_EXCL, S_IRUSR);
+ /* Try O_TMPFILE first, to avoid changing directory mtime */
+ sprintf(tmp_inode_name, "%.*s", (int)strnlen(file, PATH_MAX), file);
+ donor_fd = open64(dirname(tmp_inode_name), O_TMPFILE | O_WRONLY | O_EXCL, S_IRUSR | S_IWUSR );
? if (donor_fd < 0) {
- if (mode_flag & DETAIL) {
- PRINT_FILE_NAME(file);
- if (errno == EEXIST)
- PRINT_ERR_MSG_WITH_ERRNO(
- "File is being defraged by other program");
- else
- PRINT_ERR_MSG_WITH_ERRNO(NGMSG_FILE_OPEN);
+ sprintf(tmp_inode_name, "%.*s.defrag",
+ (int)strnlen(file, PATH_MAX), file);
+ donor_fd = open64(tmp_inode_name, O_WRONLY | O_CREAT | O_EXCL, S_IRUSR);
+ if (donor_fd < 0) {
+ if (mode_flag & DETAIL) {
+ PRINT_FILE_NAME(file);
+ if (errno == EEXIST)
+ PRINT_ERR_MSG_WITH_ERRNO(
+ "File is being defraged by other program");
+ else
+ PRINT_ERR_MSG_WITH_ERRNO(NGMSG_FILE_OPEN);
+ }
+ goto out;
? }
- goto out;
- }
?
- /* Unlink donor inode */
- ret = unlink(tmp_inode_name);
- if (ret < 0) {
- if (mode_flag & DETAIL) {
- PRINT_FILE_NAME(file);
- PRINT_ERR_MSG_WITH_ERRNO("Failed to unlink");
+ /* Unlink donor inode */
+ ret = unlink(tmp_inode_name);
+ if (ret < 0) {
+ if (mode_flag & DETAIL) {
+ PRINT_FILE_NAME(file);
+ PRINT_ERR_MSG_WITH_ERRNO("Failed to unlink");
+ }
+ goto out;
? }
- goto out;
? }
(attaching same, to fix whitespace...I suppose I should configure a proper email client sometime)
+Reardon
----------------------------------------
> From: [email protected]
> To: [email protected]
> CC: [email protected]; [email protected]
> Subject: RE: possible different donor file naming in e4defrag
> Date: Thu, 11 Sep 2014 18:49:07 -0400
>
>> On Sep 11, 2014, at 1:48 PM, TR Reardon <[email protected]> wrote:
>>> Picking this back up. How would O_TMPFILE avoid races? It definitely avoids the unwanted mtime/atime update, but then the existing "<filename>.defrag" pseudo-lock file would no longer be available. How could you use O_TMPFILE and still avoid multiple defrag? If this isn't possible, then resetting the parent times on unlink(tmpfile), as you suggest, is the simplest way out of this.
>>
>> Looking at this the opposite way - what are the chances that there
>> will be concurrent defrags on the same file? Basically no chance at
>> all. So long as it doesn't explode (the kernel would need to protect
>> against this anyway to avoid malicious apps), the worst case is that
>> there will be some extra defragmentation done in a very rare case.
>>
>> Conversely, creating a temporary filename and then resetting the
>> parent directory timestamp is extra work for every file defragmented,
>> and is racy because e4defrag may "reset" the time to before the temp
>> file was created, but clobber a legitimate timestamp update in the
>> directory from some other concurrent update. That timestamp update
>> is always going to be racy, even if e4defrag tries to be careful.
>>
>> Cheers, Andreas
>
>
> Thanks, well described.
>
> So a simple attempt with O_TMPFILE first, then revert to original behavior, like below?
>
>
> diff --git a/misc/e4defrag.c b/misc/e4defrag.c
> index d0eac60..8001182 100644
> --- a/misc/e4defrag.c
> +++ b/misc/e4defrag.c
> @@ -40,6 +40,7 @@
> #include <sys/stat.h>
> #include <sys/statfs.h>
> #include <sys/vfs.h>
> +#include <libgen.h>
>
> /* A relatively new ioctl interface ... */
> #ifndef EXT4_IOC_MOVE_EXT
> @@ -1526,31 +1527,36 @@ static int file_defrag(const char *file, const struct stat64 *buf,
>
> /* Create donor inode */
> memset(tmp_inode_name, 0, PATH_MAX + 8);
> - sprintf(tmp_inode_name, "%.*s.defrag",
> - (int)strnlen(file, PATH_MAX), file);
> - donor_fd = open64(tmp_inode_name, O_WRONLY | O_CREAT | O_EXCL, S_IRUSR);
> + /* Try O_TMPFILE first, to avoid changing directory mtime */
> + sprintf(tmp_inode_name, "%.*s", (int)strnlen(file, PATH_MAX), file);
> + donor_fd = open64(dirname(tmp_inode_name), O_TMPFILE | O_WRONLY | O_EXCL, S_IRUSR | S_IWUSR );
> if (donor_fd < 0) {
> - if (mode_flag & DETAIL) {
> - PRINT_FILE_NAME(file);
> - if (errno == EEXIST)
> - PRINT_ERR_MSG_WITH_ERRNO(
> - "File is being defraged by other program");
> - else
> - PRINT_ERR_MSG_WITH_ERRNO(NGMSG_FILE_OPEN);
> + sprintf(tmp_inode_name, "%.*s.defrag",
> + (int)strnlen(file, PATH_MAX), file);
> + donor_fd = open64(tmp_inode_name, O_WRONLY | O_CREAT | O_EXCL, S_IRUSR);
> + if (donor_fd < 0) {
> + if (mode_flag & DETAIL) {
> + PRINT_FILE_NAME(file);
> + if (errno == EEXIST)
> + PRINT_ERR_MSG_WITH_ERRNO(
> + "File is being defraged by other program");
> + else
> + PRINT_ERR_MSG_WITH_ERRNO(NGMSG_FILE_OPEN);
> + }
> + goto out;
> }
> - goto out;
> - }
>
> - /* Unlink donor inode */
> - ret = unlink(tmp_inode_name);
> - if (ret < 0) {
> - if (mode_flag & DETAIL) {
> - PRINT_FILE_NAME(file);
> - PRINT_ERR_MSG_WITH_ERRNO("Failed to unlink");
> + /* Unlink donor inode */
> + ret = unlink(tmp_inode_name);
> + if (ret < 0) {
> + if (mode_flag & DETAIL) {
> + PRINT_FILE_NAME(file);
> + PRINT_ERR_MSG_WITH_ERRNO("Failed to unlink");
> + }
> + goto out;
> }
> - goto out;
> }
> -
> +
> /* Allocate space for donor inode */
> orig_group_tmp = orig_group_head;
> do {
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sep 11, 2014, at 5:03 PM, TR Reardon <[email protected]> wrote:
> diff --git a/misc/e4defrag.c b/misc/e4defrag.c
> index d0eac60..8001182 100644
> --- a/misc/e4defrag.c
> +++ b/misc/e4defrag.c
> @@ -1526,31 +1527,36 @@ static int file_defrag(const char *file, const struct stat64 *buf,
>
> /* Create donor inode */
> memset(tmp_inode_name, 0, PATH_MAX + 8);
> - sprintf(tmp_inode_name, "%.*s.defrag",
> - (int)strnlen(file, PATH_MAX), file);
> - donor_fd = open64(tmp_inode_name, O_WRONLY | O_CREAT | O_EXCL, S_IRUSR);
> + /* Try O_TMPFILE first, to avoid changing directory mtime */
> + sprintf(tmp_inode_name, "%.*s", (int)strnlen(file, PATH_MAX), file);
> + donor_fd = open64(dirname(tmp_inode_name), O_TMPFILE | O_WRONLY | O_EXCL, S_IRUSR | S_IWUSR );
Lines need to be <= 80 columns. Please run patch through checkpatch.pl.
Why is it opened O_WRONLY, but the permissions are S_IRUSR | S_IWUSR?
I agree it didn't make sense in the old code to have S_IRUSR either,
but I don't think this makes more sense. If the file is write-only
(which is probably correct, unless e4defrag is doing some post-copy
checksum of the data) then S_IWUSR would be enough.
> if (donor_fd < 0) {
> - if (mode_flag & DETAIL) {
> - PRINT_FILE_NAME(file);
> - if (errno == EEXIST)
> - PRINT_ERR_MSG_WITH_ERRNO(
> - "File is being defraged by other program");
> - else
> - PRINT_ERR_MSG_WITH_ERRNO(NGMSG_FILE_OPEN);
> + sprintf(tmp_inode_name, "%.*s.defrag",
> + (int)strnlen(file, PATH_MAX), file);
> + donor_fd = open64(tmp_inode_name, O_WRONLY | O_CREAT | O_EXCL, S_IRUSR);
Wrap at 80 columns.
This has the same issue with O_WRONLY and S_IRUSR, though it at least
matches the old code.
> + if (donor_fd < 0) {
> + if (mode_flag & DETAIL) {
> + PRINT_FILE_NAME(file);
> + if (errno == EEXIST)
> + PRINT_ERR_MSG_WITH_ERRNO(
> + "File is being defraged by other program");
> + else
> + PRINT_ERR_MSG_WITH_ERRNO(NGMSG_FILE_OPEN);
> + }
> + goto out;
> }
> - goto out;
> - }
>
> - /* Unlink donor inode */
> - ret = unlink(tmp_inode_name);
> - if (ret < 0) {
> - if (mode_flag & DETAIL) {
> - PRINT_FILE_NAME(file);
> - PRINT_ERR_MSG_WITH_ERRNO("Failed to unlink");
> + /* Unlink donor inode */
> + ret = unlink(tmp_inode_name);
> + if (ret < 0) {
> + if (mode_flag & DETAIL) {
> + PRINT_FILE_NAME(file);
> + PRINT_ERR_MSG_WITH_ERRNO("Failed to unlink");
> + }
> + goto out;
> }
Shouldn't it reset the timestamp in this case?
Cheers, Andreas
> - goto out;
> }
> -
> +
> /* Allocate space for donor inode */
> orig_group_tmp = orig_group_head;
> do {
> ----------------------------------------
>> From: [email protected]
>> To: [email protected]
>> CC: [email protected]; [email protected]
>> Subject: RE: possible different donor file naming in e4defrag
>> Date: Thu, 11 Sep 2014 18:49:07 -0400
>>
>>> On Sep 11, 2014, at 1:48 PM, TR Reardon <[email protected]> wrote:
>>>> Picking this back up. How would O_TMPFILE avoid races? It definitely avoids the unwanted mtime/atime update, but then the existing "<filename>.defrag" pseudo-lock file would no longer be available. How could you use O_TMPFILE and still avoid multiple defrag? If this isn't possible, then resetting the parent times on unlink(tmpfile), as you suggest, is the simplest way out of this.
>>>
>>> Looking at this the opposite way - what are the chances that there
>>> will be concurrent defrags on the same file? Basically no chance at
>>> all. So long as it doesn't explode (the kernel would need to protect
>>> against this anyway to avoid malicious apps), the worst case is that
>>> there will be some extra defragmentation done in a very rare case.
>>>
>>> Conversely, creating a temporary filename and then resetting the
>>> parent directory timestamp is extra work for every file defragmented,
>>> and is racy because e4defrag may "reset" the time to before the temp
>>> file was created, but clobber a legitimate timestamp update in the
>>> directory from some other concurrent update. That timestamp update
>>> is always going to be racy, even if e4defrag tries to be careful.
>>>
>>> Cheers, Andreas
>>
>>
>> Thanks, well described.
>>
>> So a simple attempt with O_TMPFILE first, then revert to original behavior, like below?
>>
>>
>> diff --git a/misc/e4defrag.c b/misc/e4defrag.c
>> index d0eac60..8001182 100644
>> --- a/misc/e4defrag.c
>> +++ b/misc/e4defrag.c
>> @@ -40,6 +40,7 @@
>> #include <sys/stat.h>
>> #include <sys/statfs.h>
>> #include <sys/vfs.h>
>> +#include <libgen.h>
>>
>> /* A relatively new ioctl interface ... */
>> #ifndef EXT4_IOC_MOVE_EXT
>> @@ -1526,31 +1527,36 @@ static int file_defrag(const char *file, const struct stat64 *buf,
>>
>> /* Create donor inode */
>> memset(tmp_inode_name, 0, PATH_MAX + 8);
>> - sprintf(tmp_inode_name, "%.*s.defrag",
>> - (int)strnlen(file, PATH_MAX), file);
>> - donor_fd = open64(tmp_inode_name, O_WRONLY | O_CREAT | O_EXCL, S_IRUSR);
>> + /* Try O_TMPFILE first, to avoid changing directory mtime */
>> + sprintf(tmp_inode_name, "%.*s", (int)strnlen(file, PATH_MAX), file);
>> + donor_fd = open64(dirname(tmp_inode_name), O_TMPFILE | O_WRONLY | O_EXCL, S_IRUSR | S_IWUSR );
>> if (donor_fd < 0) {
>> - if (mode_flag & DETAIL) {
>> - PRINT_FILE_NAME(file);
>> - if (errno == EEXIST)
>> - PRINT_ERR_MSG_WITH_ERRNO(
>> - "File is being defraged by other program");
>> - else
>> - PRINT_ERR_MSG_WITH_ERRNO(NGMSG_FILE_OPEN);
>> + sprintf(tmp_inode_name, "%.*s.defrag",
>> + (int)strnlen(file, PATH_MAX), file);
>> + donor_fd = open64(tmp_inode_name, O_WRONLY | O_CREAT | O_EXCL, S_IRUSR);
>> + if (donor_fd < 0) {
>> + if (mode_flag & DETAIL) {
>> + PRINT_FILE_NAME(file);
>> + if (errno == EEXIST)
>> + PRINT_ERR_MSG_WITH_ERRNO(
>> + "File is being defraged by other program");
>> + else
>> + PRINT_ERR_MSG_WITH_ERRNO(NGMSG_FILE_OPEN);
>> + }
>> + goto out;
>> }
>> - goto out;
>> - }
>>
>> - /* Unlink donor inode */
>> - ret = unlink(tmp_inode_name);
>> - if (ret < 0) {
>> - if (mode_flag & DETAIL) {
>> - PRINT_FILE_NAME(file);
>> - PRINT_ERR_MSG_WITH_ERRNO("Failed to unlink");
>> + /* Unlink donor inode */
>> + ret = unlink(tmp_inode_name);
>> + if (ret < 0) {
>> + if (mode_flag & DETAIL) {
>> + PRINT_FILE_NAME(file);
>> + PRINT_ERR_MSG_WITH_ERRNO("Failed to unlink");
>> + }
>> + goto out;
>> }
>> - goto out;
>> }
>> -
>> +
>> /* Allocate space for donor inode */
>> orig_group_tmp = orig_group_head;
>> do {
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> <DEFRAG_O_TMPFILE>
Cheers, Andreas
> Subject: Re: possible different donor file naming in e4defrag
> From: [email protected]
> Date: Fri, 12 Sep 2014 13:41:17 -0600
> CC: [email protected]; [email protected]
> To: [email protected]
>
> On Sep 11, 2014, at 5:03 PM, TR Reardon <[email protected]> wrote:
>> diff --git a/misc/e4defrag.c b/misc/e4defrag.c
>> index d0eac60..8001182 100644
>> --- a/misc/e4defrag.c
>> +++ b/misc/e4defrag.c
>> @@ -1526,31 +1527,36 @@ static int file_defrag(const char *file, const struct stat64 *buf,
>>
>> /* Create donor inode */
>> memset(tmp_inode_name, 0, PATH_MAX + 8);
>> - sprintf(tmp_inode_name, "%.*s.defrag",
>> - (int)strnlen(file, PATH_MAX), file);
>> - donor_fd = open64(tmp_inode_name, O_WRONLY | O_CREAT | O_EXCL, S_IRUSR);
>> + /* Try O_TMPFILE first, to avoid changing directory mtime */
>> + sprintf(tmp_inode_name, "%.*s", (int)strnlen(file, PATH_MAX), file);
>> + donor_fd = open64(dirname(tmp_inode_name), O_TMPFILE | O_WRONLY | O_EXCL, S_IRUSR | S_IWUSR );
>
> Lines need to be <= 80 columns. Please run patch through checkpatch.pl.
>
> Why is it opened O_WRONLY, but the permissions are S_IRUSR | S_IWUSR?
> I agree it didn't make sense in the old code to have S_IRUSR either,
> but I don't think this makes more sense. If the file is write-only
> (which is probably correct, unless e4defrag is doing some post-copy
> checksum of the data) then S_IWUSR would be enough.
I agree, wasn't sure if appropriate to change existing. I have changed in updated.
>> if (donor_fd < 0) {
>> - if (mode_flag & DETAIL) {
>> - PRINT_FILE_NAME(file);
>> - if (errno == EEXIST)
>> - PRINT_ERR_MSG_WITH_ERRNO(
>> - "File is being defraged by other program");
>> - else
>> - PRINT_ERR_MSG_WITH_ERRNO(NGMSG_FILE_OPEN);
>> + sprintf(tmp_inode_name, "%.*s.defrag",
>> + (int)strnlen(file, PATH_MAX), file);
>> + donor_fd = open64(tmp_inode_name, O_WRONLY | O_CREAT | O_EXCL, S_IRUSR);
>
> Wrap at 80 columns.
>
> This has the same issue with O_WRONLY and S_IRUSR, though it at least
> matches the old code.
>
>> + if (donor_fd < 0) {
>> + if (mode_flag & DETAIL) {
>> + PRINT_FILE_NAME(file);
>> + if (errno == EEXIST)
>> + PRINT_ERR_MSG_WITH_ERRNO(
>> + "File is being defraged by other program");
>> + else
>> + PRINT_ERR_MSG_WITH_ERRNO(NGMSG_FILE_OPEN);
>> + }
>> + goto out;
>> }
>> - goto out;
>> - }
>>
>> - /* Unlink donor inode */
>> - ret = unlink(tmp_inode_name);
>> - if (ret < 0) {
>> - if (mode_flag & DETAIL) {
>> - PRINT_FILE_NAME(file);
>> - PRINT_ERR_MSG_WITH_ERRNO("Failed to unlink");
>> + /* Unlink donor inode */
>> + ret = unlink(tmp_inode_name);
>> + if (ret < 0) {
>> + if (mode_flag & DETAIL) {
>> + PRINT_FILE_NAME(file);
>> + PRINT_ERR_MSG_WITH_ERRNO("Failed to unlink");
>> + }
>> + goto out;
>> }
>
> Shouldn't it reset the timestamp in this case?
>
> Cheers, Andreas
Oh, I thought you were arguing that it should collapse to only the O_TMPFILE case to avoid unnecessary races. Updated handles it in both cases, but prefers O_TMPFILE.
(attached has proper whitespace)
--
Signed-off-by: TR Reardon <[email protected]>
--
diff --git a/misc/e4defrag.c b/misc/e4defrag.c
index d0eac60..2facf44 100644
--- a/misc/e4defrag.c
+++ b/misc/e4defrag.c
@@ -40,6 +40,7 @@
#include <sys/stat.h>
#include <sys/statfs.h>
#include <sys/vfs.h>
+#include <libgen.h>
/* A relatively new ioctl interface ... */
#ifndef EXT4_IOC_MOVE_EXT
@@ -1408,6 +1409,8 @@ static int file_defrag(const char *file, const struct stat64 *buf,
int file_frags_start, file_frags_end;
int orig_physical_cnt, donor_physical_cnt = 0;
char tmp_inode_name[PATH_MAX + 8];
+ char *parent_name = NULL;
+ struct stat parent_stat;
ext4_fsblk_t blk_count = 0;
struct fiemap_extent_list *orig_list_physical = NULL;
struct fiemap_extent_list *orig_list_logical = NULL;
@@ -1526,29 +1529,51 @@ static int file_defrag(const char *file, const struct stat64 *buf,
/* Create donor inode */
memset(tmp_inode_name, 0, PATH_MAX + 8);
- sprintf(tmp_inode_name, "%.*s.defrag",
- (int)strnlen(file, PATH_MAX), file);
- donor_fd = open64(tmp_inode_name, O_WRONLY | O_CREAT | O_EXCL, S_IRUSR);
+ /* Try O_TMPFILE first, to avoid changing directory mtime */
+ sprintf(tmp_inode_name, "%.*s", (int)strnlen(file, PATH_MAX), file);
+ parent_name = dirname(tmp_inode_name);
+ donor_fd = open64(parent_name, O_WRONLY | O_TMPFILE | O_EXCL, S_IWUSR);
if (donor_fd < 0) {
- if (mode_flag & DETAIL) {
- PRINT_FILE_NAME(file);
- if (errno == EEXIST)
+ /* Save parent timestamps for reset */
+ ret = stat(parent_name, &parent_stat);
+ if (ret < 0) {
+ if (mode_flag & DETAIL) {
+ PRINT_FILE_NAME(file);
PRINT_ERR_MSG_WITH_ERRNO(
- "File is being defraged by other program");
- else
- PRINT_ERR_MSG_WITH_ERRNO(NGMSG_FILE_OPEN);
+ "Failed to stat() parent directory");
+ }
+ goto out;
}
- goto out;
- }
- /* Unlink donor inode */
- ret = unlink(tmp_inode_name);
- if (ret < 0) {
- if (mode_flag & DETAIL) {
- PRINT_FILE_NAME(file);
- PRINT_ERR_MSG_WITH_ERRNO("Failed to unlink");
+ sprintf(tmp_inode_name, "%.*s.defrag",
+ (int)strnlen(file, PATH_MAX), file);
+ donor_fd = open64(tmp_inode_name, O_WRONLY | O_CREAT | O_EXCL,
+ S_IWUSR);
+ if (donor_fd < 0) {
+ if (mode_flag & DETAIL) {
+ PRINT_FILE_NAME(file);
+ if (errno == EEXIST)
+ PRINT_ERR_MSG_WITH_ERRNO(
+ "File is being defraged by other program");
+ else
+ PRINT_ERR_MSG_WITH_ERRNO(
+ NGMSG_FILE_OPEN);
+ }
+ goto out;
}
- goto out;
+
+ /* Unlink donor inode */
+ ret = unlink(tmp_inode_name);
+ if (ret < 0) {
+ if (mode_flag & DETAIL) {
+ PRINT_FILE_NAME(file);
+ PRINT_ERR_MSG_WITH_ERRNO("Failed to unlink");
+ }
+ goto out;
+ }
+
+ /* Reset parent times */
+ utimensat(0, parent_name, &parent_stat.st_atim, 0);
}
/* Allocate space for donor inode */