| Message ID | 1374773730-29957-5-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | Under Review, archived |
| Headers | show |
On Fri, Jul 26, 2013 at 01:35:28AM +0800, Anand Jain wrote: > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > mkfs.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/mkfs.c b/mkfs.c > index 60f906c..66f558a 100644 > --- a/mkfs.c > +++ b/mkfs.c > @@ -1570,6 +1570,8 @@ int main(int ac, char **av) > * occur by the following processing. > * (btrfs_register_one_device() fails if O_EXCL is on) > */ > + if (fd > 0) > + close(fd); > fd = open(file, O_RDWR); > if (fd < 0) { > fprintf(stderr, "unable to open %s: %s\n", file, > @@ -1581,7 +1583,6 @@ int main(int ac, char **av) > if (ret) { > fprintf(stderr, "skipping duplicate device %s in FS\n", > file); > - close(fd); > continue; > } > ret = btrfs_prepare_device(fd, file, zero_end, &dev_block_count, This breaks mkfs with multiple disks. Please for the love of all that is holy just do a xfstests run with your patches to make sure they don't break anything so when I go to try to test something completely different I don't have to waste time bisecting down to figure out wtf you broke today? David can you kick this one out of integration for the time being please? Thanks, Josef -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 13, 2013 at 03:14:08PM -0400, Josef Bacik wrote: > On Fri, Jul 26, 2013 at 01:35:28AM +0800, Anand Jain wrote: > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > > --- > > mkfs.c | 3 ++- > > 1 files changed, 2 insertions(+), 1 deletions(-) > > > > diff --git a/mkfs.c b/mkfs.c > > index 60f906c..66f558a 100644 > > --- a/mkfs.c > > +++ b/mkfs.c > > @@ -1570,6 +1570,8 @@ int main(int ac, char **av) > > * occur by the following processing. > > * (btrfs_register_one_device() fails if O_EXCL is on) > > */ > > + if (fd > 0) > > + close(fd); > > fd = open(file, O_RDWR); > > if (fd < 0) { > > fprintf(stderr, "unable to open %s: %s\n", file, > > @@ -1581,7 +1583,6 @@ int main(int ac, char **av) > > if (ret) { > > fprintf(stderr, "skipping duplicate device %s in FS\n", > > file); > > - close(fd); > > continue; > > } > > ret = btrfs_prepare_device(fd, file, zero_end, &dev_block_count, > > This breaks mkfs with multiple disks. Please for the love of all that is holy > just do a xfstests run with your patches to make sure they don't break anything > so when I go to try to test something completely different I don't have to waste > time bisecting down to figure out wtf you broke today? David can you kick this > one out of integration for the time being please? Thanks, > In fact this isn't right at all, we pass this fd into the device, so we aren't "overwriting" it, we're assigning it to the device and moving on to the next thing, and then the close_ctree() stuff closes all the devices as normal. So just no, this isn't right at all and can be evicted permanently. Thanks, Josef -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/14/2013 03:14 AM, Josef Bacik wrote: > On Fri, Jul 26, 2013 at 01:35:28AM +0800, Anand Jain wrote: >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> mkfs.c | 3 ++- >> 1 files changed, 2 insertions(+), 1 deletions(-) >> >> diff --git a/mkfs.c b/mkfs.c >> index 60f906c..66f558a 100644 >> --- a/mkfs.c >> +++ b/mkfs.c >> @@ -1570,6 +1570,8 @@ int main(int ac, char **av) >> * occur by the following processing. >> * (btrfs_register_one_device() fails if O_EXCL is on) >> */ >> + if (fd > 0) >> + close(fd); >> fd = open(file, O_RDWR); >> if (fd < 0) { >> fprintf(stderr, "unable to open %s: %s\n", file, >> @@ -1581,7 +1583,6 @@ int main(int ac, char **av) >> if (ret) { >> fprintf(stderr, "skipping duplicate device %s in FS\n", >> file); >> - close(fd); >> continue; >> } >> ret = btrfs_prepare_device(fd, file, zero_end, &dev_block_count, > > This breaks mkfs with multiple disks. I can't believe as I have been playing with multiple disks quite a lot recently. let me dig more. Anand -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/14/2013 10:04 AM, Anand Jain wrote: > > > On 08/14/2013 03:14 AM, Josef Bacik wrote: >> On Fri, Jul 26, 2013 at 01:35:28AM +0800, Anand Jain wrote: >>> Signed-off-by: Anand Jain <anand.jain@oracle.com> >>> --- >>> mkfs.c | 3 ++- >>> 1 files changed, 2 insertions(+), 1 deletions(-) >>> >>> diff --git a/mkfs.c b/mkfs.c >>> index 60f906c..66f558a 100644 >>> --- a/mkfs.c >>> +++ b/mkfs.c >>> @@ -1570,6 +1570,8 @@ int main(int ac, char **av) >>> * occur by the following processing. >>> * (btrfs_register_one_device() fails if O_EXCL is on) >>> */ >>> + if (fd > 0) >>> + close(fd); >>> fd = open(file, O_RDWR); >>> if (fd < 0) { >>> fprintf(stderr, "unable to open %s: %s\n", file, >>> @@ -1581,7 +1583,6 @@ int main(int ac, char **av) >>> if (ret) { >>> fprintf(stderr, "skipping duplicate device %s in FS\n", >>> file); >>> - close(fd); >>> continue; >>> } >>> ret = btrfs_prepare_device(fd, file, zero_end, >>> &dev_block_count, >> >> This breaks mkfs with multiple disks. > > I can't believe as I have been playing with multiple disks > quite a lot recently. let me dig more. Sorry my mistake. Indeed further down btrfs_add_to_fsid() stores fd. closing a stored fd is not correct theoretically. Josef, Would be keen to know which xfstest caught this. ? I am sending a patch to back out this, to be applied on the current integration branch if it helps users for the time being. Thanks, Anand -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 14, 2013 at 11:17:05AM +0800, Anand Jain wrote: > > > On 08/14/2013 10:04 AM, Anand Jain wrote: > > > > > >On 08/14/2013 03:14 AM, Josef Bacik wrote: > >>On Fri, Jul 26, 2013 at 01:35:28AM +0800, Anand Jain wrote: > >>>Signed-off-by: Anand Jain <anand.jain@oracle.com> > >>>--- > >>> mkfs.c | 3 ++- > >>> 1 files changed, 2 insertions(+), 1 deletions(-) > >>> > >>>diff --git a/mkfs.c b/mkfs.c > >>>index 60f906c..66f558a 100644 > >>>--- a/mkfs.c > >>>+++ b/mkfs.c > >>>@@ -1570,6 +1570,8 @@ int main(int ac, char **av) > >>> * occur by the following processing. > >>> * (btrfs_register_one_device() fails if O_EXCL is on) > >>> */ > >>>+ if (fd > 0) > >>>+ close(fd); > >>> fd = open(file, O_RDWR); > >>> if (fd < 0) { > >>> fprintf(stderr, "unable to open %s: %s\n", file, > >>>@@ -1581,7 +1583,6 @@ int main(int ac, char **av) > >>> if (ret) { > >>> fprintf(stderr, "skipping duplicate device %s in FS\n", > >>> file); > >>>- close(fd); > >>> continue; > >>> } > >>> ret = btrfs_prepare_device(fd, file, zero_end, > >>>&dev_block_count, > >> > >>This breaks mkfs with multiple disks. > > > > I can't believe as I have been playing with multiple disks > > quite a lot recently. let me dig more. > > Sorry my mistake. > > Indeed further down btrfs_add_to_fsid() stores fd. closing a > stored fd is not correct theoretically. > > Josef, Would be keen to know which xfstest caught this. ? > It was btrfs/265, the one that does raid tests and such. Josef -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/mkfs.c b/mkfs.c index 60f906c..66f558a 100644 --- a/mkfs.c +++ b/mkfs.c @@ -1570,6 +1570,8 @@ int main(int ac, char **av) * occur by the following processing. * (btrfs_register_one_device() fails if O_EXCL is on) */ + if (fd > 0) + close(fd); fd = open(file, O_RDWR); if (fd < 0) { fprintf(stderr, "unable to open %s: %s\n", file, @@ -1581,7 +1583,6 @@ int main(int ac, char **av) if (ret) { fprintf(stderr, "skipping duplicate device %s in FS\n", file); - close(fd); continue; } ret = btrfs_prepare_device(fd, file, zero_end, &dev_block_count,
Signed-off-by: Anand Jain <anand.jain@oracle.com> --- mkfs.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)