Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit mmapwrite file size #14345

Merged
merged 1 commit into from
Jan 5, 2023

Conversation

aerusso
Copy link
Contributor

@aerusso aerusso commented Jan 1, 2023

Motivation and Context

mmapwrite spawns several threads, all of which perform writes on a file for the purpose of testing the behavior of mmap(2)-ed files. One thread performs an mmap and a write to the beginning of that region, while the others perform regular writes after lseek(2)-ing the end of the file.

Because these regular writes are set in a while (1) loop, they will write an unbounded amount of data to disk. The mmap_write_001_pos test script SIGKILLs them after 30 seconds, but on fast testbeds, this may be enough time to exhaust the available space in the filesystem, leading to spurious test failures (see #14277).

Description

Limit the total file size by checking that the lseek return value from the regular write threads is no greater than 250 MB, which is less than the default minimum vdev size defined in includes/default.cfg.

How Has This Been Tested?

Submitting this to the build servers for testing, presently. I will also run this locally on a machine that exhibited the problem before.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@aerusso aerusso force-pushed the mrs/upstream/mmap-enospc branch from 2d7c895 to a09becc Compare January 1, 2023 20:04
mmapwrite spawns several threads, all of which perform writes on a file
for the purpose of testing the behavior of mmap(2)-ed files.  One
thread performs an mmap and a write to the beginning of that region,
while the others perform regular writes after lseek(2)-ing the end of
the file.

Because these regular writes are set in a while (1) loop, they will
write an unbounded amount of data to disk.  The mmap_write_001_pos test
script SIGKILLs them after 30 seconds, but on fast testbeds, this may
be enough time to exhaust the available space in the filesystem,
leading to spurious test failures.

Instead, limit the total file size by checking that the lseek return
value is no greater than 250 * 1024*1024 bytes, which is less than the
default minimum vdev size defined in includes/default.cfg .

Signed-off-by: Antonio Russo <aerusso@aerusso.net>
@aerusso aerusso force-pushed the mrs/upstream/mmap-enospc branch from a09becc to 0434c65 Compare January 1, 2023 20:08
@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Jan 5, 2023
@behlendorf behlendorf merged commit ee6bf97 into openzfs:master Jan 5, 2023
@ryao
Copy link
Contributor

ryao commented Jan 5, 2023

Coverity reports that this caused a file descriptor leak:

Hi,

Please find the latest report on new defect(s) introduced to openzfs/zfs found with Coverity Scan.

1 new defect(s) introduced to openzfs/zfs found with Coverity Scan.


New defect(s) Reported-by: Coverity Scan
Showing 1 of 1 defect(s)


** CID 1530383:  Resource leaks  (RESOURCE_LEAK)
/tests/zfs-tests/cmd/mmapwrite.c: 85 in normal_writer()


________________________________________________________________________________________________________
*** CID 1530383:  Resource leaks  (RESOURCE_LEAK)
/tests/zfs-tests/cmd/mmapwrite.c: 85 in normal_writer()
79              if ((bytes_written = lseek(fd, page_size, SEEK_CUR)) == -1) {
80                      err(1, "lseek failed on %s: %s", file_path,
81                          strerror(errno));
82                      break;
83              }
84      }
>>>     CID 1530383:  Resource leaks  (RESOURCE_LEAK)
>>>     Handle variable "fd" going out of scope leaks the handle.
85      return (NULL);
86     }
87     
88     static void *
89     map_writer(void *filename)
90     {

It is a minor issue, but we probably should fix it as we are trying to get to 0 outstanding coverity defect reports so that important issues are not hidden among a sea of minor issues. We just need close(fd); before return (NULL); to fix it.

@aerusso aerusso deleted the mrs/upstream/mmap-enospc branch January 7, 2023 02:42
aerusso added a commit to aerusso/zfs that referenced this pull request Jan 7, 2023
commit ee6bf97 upstream

mmapwrite spawns several threads, all of which perform writes on a file
for the purpose of testing the behavior of mmap(2)-ed files.  One
thread performs an mmap and a write to the beginning of that region,
while the others perform regular writes after lseek(2)-ing the end of
the file.

Because these regular writes are set in a while (1) loop, they will
write an unbounded amount of data to disk.  The mmap_write_001_pos test
script SIGKILLs them after 30 seconds, but on fast testbeds, this may
be enough time to exhaust the available space in the filesystem,
leading to spurious test failures.

Instead, limit the total file size by checking that the lseek return
value is no greater than 250 * 1024*1024 bytes, which is less than the
default minimum vdev size defined in includes/default.cfg .

This also includes part of 2a493a4,
which checks the return value of lseek.

Signed-off-by: Antonio Russo <aerusso@aerusso.net>
Closes openzfs#14277
Closes openzfs#14345
behlendorf pushed a commit that referenced this pull request Jan 10, 2023
commit ee6bf97 upstream

mmapwrite spawns several threads, all of which perform writes on a file
for the purpose of testing the behavior of mmap(2)-ed files.  One
thread performs an mmap and a write to the beginning of that region,
while the others perform regular writes after lseek(2)-ing the end of
the file.

Because these regular writes are set in a while (1) loop, they will
write an unbounded amount of data to disk.  The mmap_write_001_pos test
script SIGKILLs them after 30 seconds, but on fast testbeds, this may
be enough time to exhaust the available space in the filesystem,
leading to spurious test failures.

Instead, limit the total file size by checking that the lseek return
value is no greater than 250 * 1024*1024 bytes, which is less than the
default minimum vdev size defined in includes/default.cfg .

This also includes part of 2a493a4,
which checks the return value of lseek.

Signed-off-by: Antonio Russo <aerusso@aerusso.net>
Closes #14277
Closes #14345
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 3, 2023
mmapwrite spawns several threads, all of which perform writes on a file
for the purpose of testing the behavior of mmap(2)-ed files.  One
thread performs an mmap and a write to the beginning of that region,
while the others perform regular writes after lseek(2)-ing the end of
the file.

Because these regular writes are set in a while (1) loop, they will
write an unbounded amount of data to disk.  The mmap_write_001_pos test
script SIGKILLs them after 30 seconds, but on fast testbeds, this may
be enough time to exhaust the available space in the filesystem,
leading to spurious test failures.

Instead, limit the total file size by checking that the lseek return
value is no greater than 250 * 1024*1024 bytes, which is less than the
default minimum vdev size defined in includes/default.cfg .

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Antonio Russo <aerusso@aerusso.net>
Closes openzfs#14277 
Closes openzfs#14345
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants