> Subject: Re: possible different donor file naming in e4defrag
> From: adilger@xxxxxxxxx
> Date: Fri, 12 Sep 2014 13:41:17 -0600
> CC: darrick.wong@xxxxxxxxxx; linux-ext4@xxxxxxxxxxxxxxx
> To: thomas_reardon@xxxxxxxxxxx
>
> On Sep 11, 2014, at 5:03 PM, TR Reardon <thomas_reardon@xxxxxxxxxxx> 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 <thomas_reardon@xxxxxxxxxxx>
--
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 */ Attachment:
DEFRAG_O_TMPFILE_v2
Description: Binary data
