From patchwork Sat Sep 13 20:23:55 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: TR Reardon X-Patchwork-Id: 388963 Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 225F8140097 for ; Sun, 14 Sep 2014 06:23:58 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752030AbaIMUX5 (ORCPT ); Sat, 13 Sep 2014 16:23:57 -0400 Received: from bay004-omc3s26.hotmail.com ([65.54.190.164]:60690 "EHLO BAY004-OMC3S26.hotmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752007AbaIMUX4 (ORCPT ); Sat, 13 Sep 2014 16:23:56 -0400 Received: from BAY179-W48 ([65.54.190.188]) by BAY004-OMC3S26.hotmail.com with Microsoft SMTPSVC(7.5.7601.22724); Sat, 13 Sep 2014 13:23:56 -0700 X-TMN: [Y4mky1EMeHiJ+vB5u8EPR0gchPW06z5y] X-Originating-Email: [thomas_reardon@hotmail.com] Message-ID: From: TR Reardon To: Andreas Dilger CC: "Darrick J. Wong" , "linux-ext4@vger.kernel.org" Subject: RE: possible different donor file naming in e4defrag Date: Sat, 13 Sep 2014 16:23:55 -0400 Importance: Normal In-Reply-To: <30D261EC-6B62-4A83-BA43-29DF560DF193@dilger.ca> References: , <20140815203909.GM2808@birch.djwong.org>, , <4DF4149D-9995-475D-B25E-DAE799DE6100@dilger.ca>, , <25905DD3-CD3E-42F2-A101-715E7C205CEB@dilger.ca>, , , <30D261EC-6B62-4A83-BA43-29DF560DF193@dilger.ca> MIME-Version: 1.0 X-OriginalArrivalTime: 13 Sep 2014 20:23:56.0361 (UTC) FILETIME=[A4C46B90:01CFCF90] Sender: linux-ext4-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-ext4@vger.kernel.org > Subject: Re: possible different donor file naming in e4defrag > From: adilger@dilger.ca > Date: Fri, 12 Sep 2014 13:41:17 -0600 > CC: darrick.wong@oracle.com; linux-ext4@vger.kernel.org > To: thomas_reardon@hotmail.com > > On Sep 11, 2014, at 5:03 PM, TR Reardon 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 -- 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 #include #include +#include /* 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 */