On Wed, Jul 17, 2024 at 7:29 PM Mariusz Tkaczyk
<mariusz.tkaczyk@xxxxxxxxxxxxxxx> wrote:
>
> On Mon, 15 Jul 2024 15:35:52 +0800
> Xiao Ni <xni@xxxxxxxxxx> wrote:
>
> > Signed-off-by: Xiao Ni <xni@xxxxxxxxxx>
> > ---
> > Grow.c | 53 ++++++++++++++++++++++++++++++++++++++++-------------
> > 1 file changed, 40 insertions(+), 13 deletions(-)
> >
> > diff --git a/Grow.c b/Grow.c
> > index 7ae967bda067..632be7db8d38 100644
> > --- a/Grow.c
> > +++ b/Grow.c
> > @@ -485,6 +485,7 @@ int Grow_addbitmap(char *devname, int fd, struct context
> > *c, struct shape *s) int bitmap_fd;
> > int d;
> > int max_devs = st->max_devs;
> > + int err = 0;
> >
> > /* try to load a superblock */
> > for (d = 0; d < max_devs; d++) {
> > @@ -525,13 +526,14 @@ int Grow_addbitmap(char *devname, int fd, struct
> > context *c, struct shape *s) return 1;
> > }
> > if (ioctl(fd, SET_BITMAP_FILE, bitmap_fd) < 0) {
> > - int err = errno;
> > + err = errno;
> > if (errno == EBUSY)
> > pr_err("Cannot add bitmap while array is
> > resyncing or reshaping etc.\n"); pr_err("Cannot set bitmap file for %s: %s\n",
> > devname, strerror(err));
> > - return 1;
> > }
> > + close(bitmap_fd);
> > + return err;
>
> I don't think that we should return errno. I would say that mdadm should define
> returned statues for functions, that is why I added mdadm_status_t.
>
> Otherwise, same value may have two meanings. For example, in some cases we are
> fine with particular error code from error might be misleading (it may match
> allowed status).
> This is not the case here, it is just an example.
>
> I think that we should not return errno outside if not intended i.e. function is
> projected to return errno in every case.
Ok, I'll keep the original way.
@@ -530,8 +530,9 @@ int Grow_addbitmap(char *devname, int fd, struct
context *c, struct shape *s)
pr_err("Cannot add bitmap while array
is resyncing or reshaping etc.\n");
pr_err("Cannot set bitmap file for %s: %s\n",
devname, strerror(err));
- return 1;
}
+ close(bitmap_fd);
+ return 1;
}
>
> > }
> >
> > return 0;
> > @@ -3083,6 +3085,7 @@ static int reshape_array(char *container, int fd, char
> > *devname, int done;
> > struct mdinfo *sra = NULL;
> > char buf[SYSFS_MAX_BUF_SIZE];
> > + bool located_backup = false;
> >
> > /* when reshaping a RAID0, the component_size might be zero.
> > * So try to fix that up.
> > @@ -3165,8 +3168,10 @@ static int reshape_array(char *container, int fd, char
> > *devname, goto release;
> > }
> >
> > - if (!backup_file)
> > + if (!backup_file) {
> > backup_file = locate_backup(sra->sys_name);
> > + located_backup = true;
> > + }
> >
> > goto started;
> > }
> > @@ -3612,15 +3617,15 @@ started:
> > mdstat_wait(30 - (delayed-1) * 25);
> > } while (delayed);
> > mdstat_close();
> > - if (check_env("MDADM_GROW_VERIFY"))
> > - fd = open(devname, O_RDONLY | O_DIRECT);
> > - else
> > - fd = -1;
> > mlockall(MCL_FUTURE);
> >
> > if (signal_s(SIGTERM, catch_term) == SIG_ERR)
> > goto release;
> >
> > + if (check_env("MDADM_GROW_VERIFY"))
> > + fd = open(devname, O_RDONLY | O_DIRECT);
> > + else
> > + fd = -1;
>
> close_fd() is used unconditionally on fd few line earlier so it seems that else
> path is not needed but this code is massive so please double check if I'm right.
>
> We may call close_fd() on the closed resource and then it is not updated to -1,
> assuming that close fails with EBADF.
How about this:
- if (check_env("MDADM_GROW_VERIFY"))
- fd = open(devname, O_RDONLY | O_DIRECT);
- else
- fd = -1;
mlockall(MCL_FUTURE);
if (signal_s(SIGTERM, catch_term) == SIG_ERR)
goto release;
+ if (check_env("MDADM_GROW_VERIFY"))
+ fd = open(devname, O_RDONLY | O_DIRECT);
if (st->ss->external) {
/* metadata handler takes it from here */
done = st->ss->manage_reshape(
@@ -3632,6 +3634,7 @@ started:
fd, sra, &reshape, st, blocks, fdlist, offsets,
d - odisks, fdlist + odisks, offsets + odisks);
+ close_fd(&fd);
free(fdlist);
free(offsets);
>
> Right way to fix it is to replace all close(fd) by close_fd(fd). It will give is
> credibility that double close is not possible.
I'll replace them in the next version. But I think it should depend on
the specific case. Sometimes, we don't need to reset fd to -1, because
we don't use it anymore. So the standard close function is better.
>
>
> > if (st->ss->external) {
> > /* metadata handler takes it from here */
> > done = st->ss->manage_reshape(
> > @@ -3632,6 +3637,8 @@ started:
> > fd, sra, &reshape, st, blocks, fdlist, offsets,
> > d - odisks, fdlist + odisks, offsets + odisks);
> >
> > + if (fd >= 0)
> > + close(fd);
> > free(fdlist);
> > free(offsets);
> >
> > @@ -3701,6 +3708,8 @@ out:
> > exit(0);
> >
> > release:
> > + if (located_backup)
> > + free(backup_file);
>
> > free(fdlist);
> > free(offsets);
> > if (orig_level != UnSet && sra) {
> > @@ -3839,6 +3848,7 @@ int reshape_container(char *container, char *devname,
> > pr_err("Unable to initialize sysfs for %s\n",
> > mdstat->devnm);
> > rv = 1;
> > + close(fd);
> > break;
> > }
> >
> > @@ -4717,6 +4727,7 @@ int Grow_restart(struct supertype *st, struct mdinfo
> > *info, int *fdlist, unsigned long long *offsets;
> > unsigned long long nstripe, ostripe;
> > int ndata, odata;
> > + int fd, backup_fd = -1;
> >
> > odata = info->array.raid_disks - info->delta_disks - 1;
> > if (info->array.level == 6)
> > @@ -4732,9 +4743,18 @@ int Grow_restart(struct supertype *st, struct mdinfo
> > *info, int *fdlist,
> > * been used
> > */
> > old_disks = cnt;
> > +
> > + if (backup_file) {
> > + backup_fd = open(backup_file, O_RDONLY);
> > + if (backup_fd < 0) {
> > + pr_err("Can't open backup file %s : %s\n",
> > + backup_file, strerror(errno));
> > + return -EINVAL;
> > + }
> > + }
> > +
> > for (i=old_disks-(backup_file?1:0); i<cnt; i++) {
> > struct mdinfo dinfo;
> > - int fd;
> > int bsbsize;
> > char *devname, namebuf[20];
> > unsigned long long lo, hi;
> > @@ -4747,12 +4767,9 @@ int Grow_restart(struct supertype *st, struct mdinfo
> > *info, int *fdlist,
> > * else restore data and update all superblocks
> > */
> > if (i == old_disks-1) {
> > - fd = open(backup_file, O_RDONLY);
> > - if (fd<0) {
> > - pr_err("backup file %s inaccessible: %s\n",
> > - backup_file, strerror(errno));
> > + if (backup_fd < 0)
> > continue;
>
> You can use is_fd_valid(). Please also review other patches on that.
I searched by command `cat * | grep "< 0"`, this is the only place.
I'll change it. But is it really convenient? if_fd_valid just does the
same thing. Because if_fd_valid is not a standard C api, developers
may not know this api.
>
> > - }
> > + fd = backup_fd;
> > devname = backup_file;
> > } else {
> > fd = fdlist[i];
> > @@ -4907,6 +4924,8 @@ int Grow_restart(struct supertype *st, struct mdinfo
> > *info, int *fdlist, pr_err("Error restoring backup from %s\n",
> > devname);
> > free(offsets);
> > + if (backup_fd >= 0)
> > + close(backup_fd);
> we have close_fd() for that.
>
> > return 1;
> > }
> >
> > @@ -4923,6 +4942,8 @@ int Grow_restart(struct supertype *st, struct mdinfo
> > *info, int *fdlist, pr_err("Error restoring second backup from %s\n",
> > devname);
> > free(offsets);
> > + if (backup_fd >= 0)
> > + close(backup_fd);
>
> You can use close_fd(). Also please analyze other patches on that.
Ok, I will do it.
Best Regards
Xiao
>
> > return 1;
> > }
> >
> > @@ -4984,8 +5005,14 @@ int Grow_restart(struct supertype *st, struct mdinfo
> > *info, int *fdlist, st->ss->store_super(st, fdlist[j]);
> > st->ss->free_super(st);
> > }
> > + if (backup_fd >= 0)
> > + close(backup_fd);
> > return 0;
> > }
> > +
> > + if (backup_fd >= 0)
> > + close(backup_fd);
>
> As above (use close_fd())
> > +
> > /* Didn't find any backup data, try to see if any
> > * was needed.
> > */
>
> Thanks,
> Mariusz
>
