| Message ID | 20191218131533.15323-1-mdoucha@suse.cz |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] Use real FS block size in fallocate05 | expand |
----- Original Message ----- > fallocate() behavior depends on whether the file range is aligned to full > blocks. Make sure that the test always uses aligned file range so that > the test is consistent across platforms. > > Also use the TEST() macro to prevent errno pollution and increase test device > size to avoid weird edge cases that don't happen in the real world. > > Signed-off-by: Martin Doucha <mdoucha@suse.cz> Acked-by: Jan Stancek <jstancek@redhat.com>
Hi! > Changes since v1: > - XFS keeps some free blocks even when write() failed with ENOSPC. Repeat > fallocate() until it gets ENOSPC, too. > - Deallocate only part of the file. > - Add description of test scenario in the header comment. > - Increase test device size to 1GB to avoid unrealistic Btrfs edge cases. Do we really need 1GB here? That quadruples the runtime. Aren't we good with just 512MB, that would just double it? > Changes since v2: > - Deallocate whole file on Btrfs, otherwise the PUNCH_HOLE check will fail. > Btrfs deallocates only complete file extents by design. > > .../kernel/syscalls/fallocate/fallocate05.c | 116 ++++++++++++++---- > 1 file changed, 89 insertions(+), 27 deletions(-) > > diff --git a/testcases/kernel/syscalls/fallocate/fallocate05.c b/testcases/kernel/syscalls/fallocate/fallocate05.c > index 17034e5b1..34faabbe8 100644 > --- a/testcases/kernel/syscalls/fallocate/fallocate05.c > +++ b/testcases/kernel/syscalls/fallocate/fallocate05.c > @@ -1,75 +1,134 @@ > // SPDX-License-Identifier: GPL-2.0-or-later > /* > * Copyright (c) 2017 Cyril Hrubis <chrubis@suse.cz> > + * Copyright (c) 2019 SUSE LLC <mdoucha@suse.cz> > */ > > /* > * Tests that writing to fallocated file works when filesystem is full. > + * Test scenario: > + * - fallocate() some empty blocks > + * - fill the filesystem > + * - write() into the preallocated space > + * - try to fallocate() more blocks until we get ENOSPC > + * - write() into the extra allocated space > + * - deallocate part of the file > + * - write() to the end of file to check that some blocks were freed > */ > > #define _GNU_SOURCE > > #include <stdio.h> > #include <stdlib.h> > -#include <errno.h> > +#include <string.h> > #include <fcntl.h> > #include "tst_test.h" > #include "lapi/fallocate.h" > > #define MNTPOINT "mntpoint" > -#define FALLOCATE_SIZE (1024*1024) > +#define FALLOCATE_BLOCKS 16 > +#define DEALLOCATE_BLOCKS 4 > #define TESTED_FLAGS "fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE)" > > static int fd; > +static char *buf = NULL; There is no point in setting global variable to zero/NULL. > static void run(void) > { > - char buf[FALLOCATE_SIZE]; > - ssize_t ret; > + long bufsize, extsize, tmp; > + blksize_t blocksize; > + struct stat statbuf; > > fd = SAFE_OPEN(MNTPOINT "/test_file", O_WRONLY | O_CREAT); > > - if (fallocate(fd, 0, 0, FALLOCATE_SIZE)) { > - if (errno == EOPNOTSUPP) { > - tst_res(TCONF | TERRNO, "fallocate() not supported"); > + /* > + * Use real FS block size, otherwise fallocate() call will test > + * different things on different platforms > + */ > + SAFE_FSTAT(fd, &statbuf); > + blocksize = statbuf.st_blksize; > + bufsize = FALLOCATE_BLOCKS * blocksize; > + buf = realloc(buf, bufsize); > + > + if (!buf) { > + SAFE_CLOSE(fd); > + tst_brk(TBROK, "Buffer allocation failed"); > + } Why realloc()? Each filesystem is tested in separately forked process so buf can't be anything but NULL here. So this should just simply be SAFE_MALLOC() and this piece of code, the part that gets the blocksize and allocates the buffer should be moved into the test setup() function that is executed also once per filesystem. And the free should be in the test cleanup(). That way we would allocate the buffer only once if the test was executed with -i option. > + TEST(fallocate(fd, 0, 0, bufsize)); > + > + if (TST_RET) { > + if (TST_ERR == ENOTSUP) { > SAFE_CLOSE(fd); > - return; > + tst_brk(TCONF | TTERRNO, "fallocate() not supported"); > } > > - tst_brk(TBROK | TERRNO, > - "fallocate(fd, 0, 0, %i)", FALLOCATE_SIZE); > + tst_brk(TBROK | TTERRNO, "fallocate(fd, 0, 0, %ld)", bufsize); > } > > tst_fill_fs(MNTPOINT, 1); > > - ret = write(fd, buf, sizeof(buf)); > + TEST(write(fd, buf, bufsize)); > > - if (ret < 0) > - tst_res(TFAIL | TERRNO, "write() failed unexpectedly"); > + if (TST_RET < 0) > + tst_res(TFAIL | TTERRNO, "write() failed unexpectedly"); > + else if (TST_RET != bufsize) > + tst_res(TFAIL, > + "Short write(): %ld bytes (expected %zu)", > + TST_RET, bufsize); > else > - tst_res(TPASS, "write() wrote %zu bytes", ret); > + tst_res(TPASS, "write() wrote %ld bytes", TST_RET); > + > + /* > + * Some file systems may still have a few extra blocks that can be > + * allocated. > + */ > + for (TST_RET = 0, extsize = 0; !TST_RET; extsize += blocksize) { > + TEST(fallocate(fd, 0, bufsize + extsize, blocksize)); > + } This is minor, but LKML prefers not to have curly braces around single line blocks. > + if (TST_RET != -1) > + tst_brk(TFAIL, "Invalid fallocate() return value %ld", > + TST_RET); Isn't this line under 80 chars even with the TST_RET); at the end? > - ret = fallocate(fd, 0, FALLOCATE_SIZE, FALLOCATE_SIZE); > - if (ret != -1) > - tst_brk(TFAIL, "fallocate() succeeded unexpectedly"); > + if (TST_ERR != ENOSPC) > + tst_brk(TFAIL | TTERRNO, "fallocate() should fail with ENOSPC"); > > - if (errno != ENOSPC) > - tst_brk(TFAIL | TERRNO, "fallocate() should fail with ENOSPC"); > + /* The loop above always counts 1 more block than it should. */ > + extsize -= blocksize; > + tst_res(TINFO, "fallocate()d %ld extra blocks on full FS", > + extsize / blocksize); > > - tst_res(TPASS | TERRNO, "fallocate() on full FS"); > + for (tmp = extsize; tmp > 0; tmp -= TST_RET) { > + TEST(write(fd, buf, MIN(bufsize, tmp))); > > - ret = fallocate(fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, 0, FALLOCATE_SIZE); > - if (ret == -1) { > - if (errno == EOPNOTSUPP) > + if (TST_RET <= 0) > + tst_brk(TFAIL | TTERRNO, "write() failed unexpectedly"); tst_brk(TFAIL, is not allowed at the moment, see: https://github.com/linux-test-project/ltp/issues/462 The only current solution is to tst_res() + return Also shouldn't we check for the write size here as well? > + } > + > + tst_res(TPASS, "fallocate() on full FS"); > + > + /* Btrfs deallocates only complete extents, not individual blocks */ > + if (!strcmp(tst_device->fs_type, "btrfs")) { > + tmp = bufsize + extsize; > + } else { > + tmp = DEALLOCATE_BLOCKS * blocksize; > + } > + > + TEST(fallocate(fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, 0, > + tmp)); Here as well, isn't the line under 80 chars? Other than these minor things the changes looks good to me.
On 1/7/20 4:21 PM, Cyril Hrubis wrote: > Hi! >> Changes since v1: >> - Increase test device size to 1GB to avoid unrealistic Btrfs edge cases. > > Do we really need 1GB here? That quadruples the runtime. Aren't we good > with just 512MB, that would just double it? I guess that's a question for Btrfs devs, so let's ask them. We're trying to test fallocate()/write() on various file systems (both space allocation and deallocation). What's the minimum block device size where Btrfs will use the same code paths as in real-world use cases? mkfs.btrfs is called without --mixed.
On 1/7/20 4:21 PM, Cyril Hrubis wrote: >> + bufsize = FALLOCATE_BLOCKS * blocksize; >> + buf = realloc(buf, bufsize); >> + >> + if (!buf) { >> + SAFE_CLOSE(fd); >> + tst_brk(TBROK, "Buffer allocation failed"); >> + } > > Why realloc()? Each filesystem is tested in separately forked process so > buf can't be anything but NULL here. > > So this should just simply be SAFE_MALLOC() and this piece of code, the > part that gets the blocksize and allocates the buffer should be moved > into the test setup() function that is executed also once per > filesystem. And the free should be in the test cleanup(). > > That way we would allocate the buffer only once if the test was executed > with -i option. Where is this control flow documented? When some behavior is not documented, I assume it may change without notice and write my code so that it will work in every case. >> - tst_res(TPASS | TERRNO, "fallocate() on full FS"); >> + for (tmp = extsize; tmp > 0; tmp -= TST_RET) { >> + TEST(write(fd, buf, MIN(bufsize, tmp))); >> >> - ret = fallocate(fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, 0, FALLOCATE_SIZE); >> - if (ret == -1) { >> - if (errno == EOPNOTSUPP) >> + if (TST_RET <= 0) >> + tst_brk(TFAIL | TTERRNO, "write() failed unexpectedly"); > > tst_brk(TFAIL, is not allowed at the moment, see: > > https://github.com/linux-test-project/ltp/issues/462 > > The only current solution is to tst_res() + return > > Also shouldn't we check for the write size here as well? I'll fix the tst_brk(). The code above will either fill the extra allocated space to the last byte, or hit the tst_brk(). No other result is possible. I don't want to pedantically check for short writes because we're not testing write() here. I'll implement the rest of your suggestions and resubmit when we get a reply from Btrfs devs.
Hi! > > Why realloc()? Each filesystem is tested in separately forked process so > > buf can't be anything but NULL here. > > > > So this should just simply be SAFE_MALLOC() and this piece of code, the > > part that gets the blocksize and allocates the buffer should be moved > > into the test setup() function that is executed also once per > > filesystem. And the free should be in the test cleanup(). > > > > That way we would allocate the buffer only once if the test was executed > > with -i option. > > Where is this control flow documented? When some behavior is not > documented, I assume it may change without notice and write my code so > that it will work in every case. Unfortunately at the moment only in the lib/tst_test.c source code. I want to write down a design document for the library, that would explain the more complicated parts and decisions, but I'm not sure when I will get to that. > >> - tst_res(TPASS | TERRNO, "fallocate() on full FS"); > >> + for (tmp = extsize; tmp > 0; tmp -= TST_RET) { > >> + TEST(write(fd, buf, MIN(bufsize, tmp))); > >> > >> - ret = fallocate(fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, 0, FALLOCATE_SIZE); > >> - if (ret == -1) { > >> - if (errno == EOPNOTSUPP) > >> + if (TST_RET <= 0) > >> + tst_brk(TFAIL | TTERRNO, "write() failed unexpectedly"); > > > > tst_brk(TFAIL, is not allowed at the moment, see: > > > > https://github.com/linux-test-project/ltp/issues/462 > > > > The only current solution is to tst_res() + return > > > > Also shouldn't we check for the write size here as well? > > I'll fix the tst_brk(). > > The code above will either fill the extra allocated space to the last > byte, or hit the tst_brk(). No other result is possible. I don't want to > pedantically check for short writes because we're not testing write() here. > > I'll implement the rest of your suggestions and resubmit when we get a > reply from Btrfs devs. Ok, thanks.
Hello, dear Btrfs devs, we know that you're busy fixing bugs and implementing new features but could you spare a minute to answer a question that'll help us improve Btrfs test coverage in LTP? We'd like to include the improved fallocate() test in the new LTP release which is planned for early next week. See the question below: On 1/7/20 4:50 PM, Martin Doucha wrote: > On 1/7/20 4:21 PM, Cyril Hrubis wrote: >> Hi! >>> Changes since v1: >>> - Increase test device size to 1GB to avoid unrealistic Btrfs edge cases. >> >> Do we really need 1GB here? That quadruples the runtime. Aren't we good >> with just 512MB, that would just double it? > > I guess that's a question for Btrfs devs, so let's ask them. > > We're trying to test fallocate()/write() on various file systems (both > space allocation and deallocation). What's the minimum block device size > where Btrfs will use the same code paths as in real-world use cases? > mkfs.btrfs is called without --mixed.
On 2020/1/13 下午8:16, Martin Doucha wrote: > Hello, dear Btrfs devs, > we know that you're busy fixing bugs and implementing new features but > could you spare a minute to answer a question that'll help us improve > Btrfs test coverage in LTP? > > We'd like to include the improved fallocate() test in the new LTP > release which is planned for early next week. See the question below: > > On 1/7/20 4:50 PM, Martin Doucha wrote: >> On 1/7/20 4:21 PM, Cyril Hrubis wrote: >>> Hi! >>>> Changes since v1: >>>> - Increase test device size to 1GB to avoid unrealistic Btrfs edge cases. >>> >>> Do we really need 1GB here? That quadruples the runtime. Aren't we good >>> with just 512MB, that would just double it? >> >> I guess that's a question for Btrfs devs, so let's ask them. >> >> We're trying to test fallocate()/write() on various file systems (both >> space allocation and deallocation). Just a small tip, btrfs defaults to data CoW, and unlike other CoW fs (like xfs), btrfs has an extent booking behavior, that even only part of a large extent (e.g 128MiB) is referred, the whole extent will not be freed. So for the following case, it may cause problem for used space accounting: # xfs_io -f -c "fallocate 0 128M" -c sync -c "fpunch 0 127M" \ /mnt/btrfs/file1 That 128M will still be considered as used, that 127M punch won't free any space. >> What's the minimum block device size >> where Btrfs will use the same code paths as in real-world use cases? Mkfs.btrfs no longer enables --mixed for small fs. But btrfs still has a pretty complex minimal device size, it depends on profile (-m and -d options). If LTP guys want to be safe for single device, it needs 256MiB for `mkfs.btrfs -m dup -d dup` to run successfully. If only default case (-m dup -d single) is needed, then 128MiB is enough. Thanks, Qu >> mkfs.btrfs is called without --mixed. >
On 1/13/20 2:16 PM, Qu WenRuo wrote: > Just a small tip, btrfs defaults to data CoW, and unlike other CoW fs > (like xfs), btrfs has an extent booking behavior, that even only part of > a large extent (e.g 128MiB) is referred, the whole extent will not be freed. I know, I reported the bug where we discussed this. >>> What's the minimum block device size >>> where Btrfs will use the same code paths as in real-world use cases? > > Mkfs.btrfs no longer enables --mixed for small fs. > > But btrfs still has a pretty complex minimal device size, it depends on > profile (-m and -d options). > > If LTP guys want to be safe for single device, it needs 256MiB for > `mkfs.btrfs -m dup -d dup` to run successfully. > > If only default case (-m dup -d single) is needed, then 128MiB is enough. Sorry but my question was not about the minimum for mkfs. My question was about the minimum device size so that the kernel driver will use the same block allocation logic as on a 100GB+ partition (instead of some special case allocation logic for tiny block devices).
On 2020/1/13 下午9:25, Martin Doucha wrote: > On 1/13/20 2:16 PM, Qu WenRuo wrote: >> Just a small tip, btrfs defaults to data CoW, and unlike other CoW fs >> (like xfs), btrfs has an extent booking behavior, that even only part of >> a large extent (e.g 128MiB) is referred, the whole extent will not be freed. > > I know, I reported the bug where we discussed this. > >>>> What's the minimum block device size >>>> where Btrfs will use the same code paths as in real-world use cases? >> >> Mkfs.btrfs no longer enables --mixed for small fs. >> >> But btrfs still has a pretty complex minimal device size, it depends on >> profile (-m and -d options). >> >> If LTP guys want to be safe for single device, it needs 256MiB for >> `mkfs.btrfs -m dup -d dup` to run successfully. >> >> If only default case (-m dup -d single) is needed, then 128MiB is enough. > > Sorry but my question was not about the minimum for mkfs. My question > was about the minimum device size so that the kernel driver will use the > same block allocation logic as on a 100GB+ partition (instead of some > special case allocation logic for tiny block devices). Then the meaningful boundary is 50G. Smaller 50G, metadata chunk is limited to 256M, otherwise metadata chunk can be 1G sized. Despite chunk size, there shouldn't be much difference in the code path. I guess you're talking about the old `mixed` behavior, which is no longer the default option for any fs size. Thanks, Qu
diff --git a/testcases/kernel/syscalls/fallocate/fallocate05.c b/testcases/kernel/syscalls/fallocate/fallocate05.c index 17034e5b1..34faabbe8 100644 --- a/testcases/kernel/syscalls/fallocate/fallocate05.c +++ b/testcases/kernel/syscalls/fallocate/fallocate05.c @@ -1,75 +1,134 @@ // SPDX-License-Identifier: GPL-2.0-or-later /* * Copyright (c) 2017 Cyril Hrubis <chrubis@suse.cz> + * Copyright (c) 2019 SUSE LLC <mdoucha@suse.cz> */ /* * Tests that writing to fallocated file works when filesystem is full. + * Test scenario: + * - fallocate() some empty blocks + * - fill the filesystem + * - write() into the preallocated space + * - try to fallocate() more blocks until we get ENOSPC + * - write() into the extra allocated space + * - deallocate part of the file + * - write() to the end of file to check that some blocks were freed */ #define _GNU_SOURCE #include <stdio.h> #include <stdlib.h> -#include <errno.h> +#include <string.h> #include <fcntl.h> #include "tst_test.h" #include "lapi/fallocate.h" #define MNTPOINT "mntpoint" -#define FALLOCATE_SIZE (1024*1024) +#define FALLOCATE_BLOCKS 16 +#define DEALLOCATE_BLOCKS 4 #define TESTED_FLAGS "fallocate(FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE)" static int fd; +static char *buf = NULL; static void run(void) { - char buf[FALLOCATE_SIZE]; - ssize_t ret; + long bufsize, extsize, tmp; + blksize_t blocksize; + struct stat statbuf; fd = SAFE_OPEN(MNTPOINT "/test_file", O_WRONLY | O_CREAT); - if (fallocate(fd, 0, 0, FALLOCATE_SIZE)) { - if (errno == EOPNOTSUPP) { - tst_res(TCONF | TERRNO, "fallocate() not supported"); + /* + * Use real FS block size, otherwise fallocate() call will test + * different things on different platforms + */ + SAFE_FSTAT(fd, &statbuf); + blocksize = statbuf.st_blksize; + bufsize = FALLOCATE_BLOCKS * blocksize; + buf = realloc(buf, bufsize); + + if (!buf) { + SAFE_CLOSE(fd); + tst_brk(TBROK, "Buffer allocation failed"); + } + + TEST(fallocate(fd, 0, 0, bufsize)); + + if (TST_RET) { + if (TST_ERR == ENOTSUP) { SAFE_CLOSE(fd); - return; + tst_brk(TCONF | TTERRNO, "fallocate() not supported"); } - tst_brk(TBROK | TERRNO, - "fallocate(fd, 0, 0, %i)", FALLOCATE_SIZE); + tst_brk(TBROK | TTERRNO, "fallocate(fd, 0, 0, %ld)", bufsize); } tst_fill_fs(MNTPOINT, 1); - ret = write(fd, buf, sizeof(buf)); + TEST(write(fd, buf, bufsize)); - if (ret < 0) - tst_res(TFAIL | TERRNO, "write() failed unexpectedly"); + if (TST_RET < 0) + tst_res(TFAIL | TTERRNO, "write() failed unexpectedly"); + else if (TST_RET != bufsize) + tst_res(TFAIL, + "Short write(): %ld bytes (expected %zu)", + TST_RET, bufsize); else - tst_res(TPASS, "write() wrote %zu bytes", ret); + tst_res(TPASS, "write() wrote %ld bytes", TST_RET); + + /* + * Some file systems may still have a few extra blocks that can be + * allocated. + */ + for (TST_RET = 0, extsize = 0; !TST_RET; extsize += blocksize) { + TEST(fallocate(fd, 0, bufsize + extsize, blocksize)); + } + + if (TST_RET != -1) + tst_brk(TFAIL, "Invalid fallocate() return value %ld", + TST_RET); - ret = fallocate(fd, 0, FALLOCATE_SIZE, FALLOCATE_SIZE); - if (ret != -1) - tst_brk(TFAIL, "fallocate() succeeded unexpectedly"); + if (TST_ERR != ENOSPC) + tst_brk(TFAIL | TTERRNO, "fallocate() should fail with ENOSPC"); - if (errno != ENOSPC) - tst_brk(TFAIL | TERRNO, "fallocate() should fail with ENOSPC"); + /* The loop above always counts 1 more block than it should. */ + extsize -= blocksize; + tst_res(TINFO, "fallocate()d %ld extra blocks on full FS", + extsize / blocksize); - tst_res(TPASS | TERRNO, "fallocate() on full FS"); + for (tmp = extsize; tmp > 0; tmp -= TST_RET) { + TEST(write(fd, buf, MIN(bufsize, tmp))); - ret = fallocate(fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, 0, FALLOCATE_SIZE); - if (ret == -1) { - if (errno == EOPNOTSUPP) + if (TST_RET <= 0) + tst_brk(TFAIL | TTERRNO, "write() failed unexpectedly"); + } + + tst_res(TPASS, "fallocate() on full FS"); + + /* Btrfs deallocates only complete extents, not individual blocks */ + if (!strcmp(tst_device->fs_type, "btrfs")) { + tmp = bufsize + extsize; + } else { + tmp = DEALLOCATE_BLOCKS * blocksize; + } + + TEST(fallocate(fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, 0, + tmp)); + + if (TST_RET == -1) { + if (TST_ERR == ENOTSUP) tst_brk(TCONF, TESTED_FLAGS); - tst_brk(TBROK | TERRNO, TESTED_FLAGS); + tst_brk(TBROK | TTERRNO, TESTED_FLAGS); } tst_res(TPASS, TESTED_FLAGS); - ret = write(fd, buf, 10); - if (ret == -1) - tst_res(TFAIL | TERRNO, "write()"); + TEST(write(fd, buf, 10)); + if (TST_RET == -1) + tst_res(TFAIL | TTERRNO, "write()"); else tst_res(TPASS, "write()"); @@ -80,12 +139,15 @@ static void cleanup(void) { if (fd > 0) SAFE_CLOSE(fd); + + free(buf); } static struct tst_test test = { .needs_root = 1, .needs_tmpdir = 1, .mount_device = 1, + .dev_min_size = 1024, .mntpoint = MNTPOINT, .all_filesystems = 1, .cleanup = cleanup,
fallocate() behavior depends on whether the file range is aligned to full blocks. Make sure that the test always uses aligned file range so that the test is consistent across platforms. Also use the TEST() macro to prevent errno pollution and increase test device size to avoid weird edge cases that don't happen in the real world. Signed-off-by: Martin Doucha <mdoucha@suse.cz> --- Using fixed-size buffer in fallocate05 caused some failures in the past due to allocation requests being misaligned with actual file system blocks. Btrfs in particular will treat misaligned allocation as regular write() and apply copy-on-write to partially allocated blocks even on the first real write(). While that behavior is somewhat surprising, it does make sense. Fix the error by using multiples of real block size in fallocate() and write(). I'll also write another fallocate() test later for checking FS behavior on intentionally misaligned allocation. But this fix can be committed before that. Changes since v1: - XFS keeps some free blocks even when write() failed with ENOSPC. Repeat fallocate() until it gets ENOSPC, too. - Deallocate only part of the file. - Add description of test scenario in the header comment. - Increase test device size to 1GB to avoid unrealistic Btrfs edge cases. Changes since v2: - Deallocate whole file on Btrfs, otherwise the PUNCH_HOLE check will fail. Btrfs deallocates only complete file extents by design. .../kernel/syscalls/fallocate/fallocate05.c | 116 ++++++++++++++---- 1 file changed, 89 insertions(+), 27 deletions(-)